Message ID | 20230412094235.589089-4-yoong.siang.song@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp202466vqo; Wed, 12 Apr 2023 03:02:27 -0700 (PDT) X-Google-Smtp-Source: AKy350Z54OFQPVqAUo9Roaqf4r1tmmtyT6Bm5UKx2BwMZgHhp/t5sq35lcDLSds4VlUYI1VQ0LmQ X-Received: by 2002:a05:6402:489:b0:4fb:2296:30af with SMTP id k9-20020a056402048900b004fb229630afmr13862027edv.20.1681293747161; Wed, 12 Apr 2023 03:02:27 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681293747; cv=none; d=google.com; s=arc-20160816; b=CqJ5t+LuiEX0/stBI1J7K3nynED1KGTjzWlQp1nIokRtUntVCZH9ZzenBR190QDMO1 acAjNxqnfJwrMrHl/SQqkdfYCgcL425FoCF5t4aQtSxTN6HGIfJJXGsjUsKBMmdMiaxs a7O0T2wn390NR9aKmA8v/CD9idoL84hIm0THZy2IGeqHB55mbakOwFWdOUUIXyB2UfDs Y6+GB2jjZO0abMNYAYbO9Y+qdvWGn2rvvCaU7pLFnK0+IDDbp7scYRge0zRyeK95Y/Jc 7RHr/lyPjDbzYVi0g0XrM/uZLErwDbq2o2TkyzoxgewBX1h0B+bM/riOSowUq/mLhy1P wW3w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=jYo017QjIxazGMHsmZKMLdqv8fXlOO9WGIDbRWqWgNs=; b=nHmpX6wyZL0ssNCVJrlB0iXuHjFYJ0T25nPSf+agBdhooVzVKJdFWwa3+u9EePfSrk JGCEoWeeEyUQSS7hco7Ld13c4U9zUH9t0McHoCugxypF2sVGsU5N7jlA8O8FcjN3ClkD gdIkBlJAfGdBHf/sRCOjX/xvo7xg1TxkynvRADd2sNJv9qmbGtmEcVX5iQVC0S/IUeiL F4ynhx94j6ELmRwuw9A1+JxB0/SJXG4voYtRvWBaySSS5tzh0qaLVhN6jflHEPsszHvF SL9EoeUhWHNxI0XiCF1pEKbG/Xw8XoBoQ0w6AL0Px45kYK3jEwWZbwOXgAqfzeMR/Jf8 4oaw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=BSyv0eOE; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u8-20020a50eac8000000b0050229538ebcsi121509edp.509.2023.04.12.03.01.59; Wed, 12 Apr 2023 03:02:27 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=BSyv0eOE; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231652AbjDLJoB (ORCPT <rfc822;peter110.wang@gmail.com> + 99 others); Wed, 12 Apr 2023 05:44:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59752 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231624AbjDLJns (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 12 Apr 2023 05:43:48 -0400 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EF1278691; Wed, 12 Apr 2023 02:43:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1681292616; x=1712828616; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=mhGhk0aFNDU+nvi/Zr0WQsY6I/0lnGV4GKGpzNN0N7A=; b=BSyv0eOEt3UXL2jf3IBcc9FBTfPmmjHBAqOuptMPOHxhWNnDIp2NtxnW db7eNNM6PCOH+qhyAuaWJaY4ijjVtwlODFa+JnzFxnKwAitamWhTNS12Y QY7meXqSoJa1Yvtz0BKCsEAk/HVzpqk4Fz8yRnoA7DUrd7GVVWFaxoSBO mbRhoeEJaC271MpPNTeQSztZAOorXjeqfvYNsyR1ticWucFlpPdrImfON Ux5uWaazbC99GjMC57/N2sSpmBUhx6oJjB2DFoZWxQWZAmGjnD3h/LDaz YCHBtKUvmnAPeA9WoS9LCC6+9lgIJtaZLazPlAo21pyDsQ/OxAO1DHptm w==; X-IronPort-AV: E=McAfee;i="6600,9927,10677"; a="346526426" X-IronPort-AV: E=Sophos;i="5.98,338,1673942400"; d="scan'208";a="346526426" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Apr 2023 02:43:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10677"; a="682430956" X-IronPort-AV: E=Sophos;i="5.98,338,1673942400"; d="scan'208";a="682430956" Received: from p12ill20yoongsia.png.intel.com ([10.88.227.28]) by orsmga007.jf.intel.com with ESMTP; 12 Apr 2023 02:43:30 -0700 From: Song Yoong Siang <yoong.siang.song@intel.com> To: Giuseppe Cavallaro <peppe.cavallaro@st.com>, Alexandre Torgue <alexandre.torgue@foss.st.com>, Jose Abreu <joabreu@synopsys.com>, "David S . Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Jesper Dangaard Brouer <hawk@kernel.org>, John Fastabend <john.fastabend@gmail.com>, Stanislav Fomichev <sdf@google.com>, Alexander Duyck <alexanderduyck@fb.com>, Ong Boon Leong <boon.leong.ong@intel.com> Cc: netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, bpf@vger.kernel.org, xdp-hints@xdp-project.net, Song Yoong Siang <yoong.siang.song@intel.com> Subject: [PATCH net-next v3 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt Date: Wed, 12 Apr 2023 17:42:34 +0800 Message-Id: <20230412094235.589089-4-yoong.siang.song@intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230412094235.589089-1-yoong.siang.song@intel.com> References: <20230412094235.589089-1-yoong.siang.song@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1762964271913687983?= X-GMAIL-MSGID: =?utf-8?q?1762964271913687983?= |
Series |
XDP Rx HWTS metadata for stmmac driver
|
|
Commit Message
Song Yoong Siang
April 12, 2023, 9:42 a.m. UTC
Add receive hardware timestamp metadata support via kfunc to XDP receive
packets.
Suggested-by: Stanislav Fomichev <sdf@google.com>
Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 ++++++++++++++++++-
2 files changed, 28 insertions(+), 1 deletion(-)
Comments
On 04/12, Song Yoong Siang wrote: > Add receive hardware timestamp metadata support via kfunc to XDP receive > packets. > > Suggested-by: Stanislav Fomichev <sdf@google.com> > Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 ++++++++++++++++++- > 2 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index ac8ccf851708..826ac0ec88c6 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -94,6 +94,9 @@ struct stmmac_rx_buffer { > > struct stmmac_xdp_buff { > struct xdp_buff xdp; > + struct stmmac_priv *priv; > + struct dma_desc *p; > + struct dma_desc *np; > }; > > struct stmmac_rx_queue { > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index f7bbdf04d20c..ed660927b628 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -5315,10 +5315,15 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) > > xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); > xdp_prepare_buff(&ctx.xdp, page_address(buf->page), > - buf->page_offset, buf1_len, false); > + buf->page_offset, buf1_len, true); > > pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - > buf->page_offset; > + > + ctx.priv = priv; > + ctx.p = p; > + ctx.np = np; > + > skb = stmmac_xdp_run_prog(priv, &ctx.xdp); > /* Due xdp_adjust_tail: DMA sync for_device > * cover max len CPU touch > @@ -7071,6 +7076,23 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) > } > } > > +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 *timestamp) > +{ > + const struct stmmac_xdp_buff *ctx = (void *)_ctx; > + > + *timestamp = 0; > + stmmac_get_rx_hwtstamp(ctx->priv, ctx->p, ctx->np, timestamp); > + [..] > + if (*timestamp) Nit: does it make sense to change stmmac_get_rx_hwtstamp to return bool to indicate success/failure? Then you can do: if (!stmmac_get_rx_hwtstamp()) reutrn -ENODATA;
On 4/12/2023 10:00 AM, Stanislav Fomichev wrote: > On 04/12, Song Yoong Siang wrote: >> Add receive hardware timestamp metadata support via kfunc to XDP receive >> packets. >> >> Suggested-by: Stanislav Fomichev <sdf@google.com> >> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ >> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 ++++++++++++++++++- >> 2 files changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> index ac8ccf851708..826ac0ec88c6 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> @@ -94,6 +94,9 @@ struct stmmac_rx_buffer { >> >> struct stmmac_xdp_buff { >> struct xdp_buff xdp; >> + struct stmmac_priv *priv; >> + struct dma_desc *p; >> + struct dma_desc *np; >> }; >> >> struct stmmac_rx_queue { >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> index f7bbdf04d20c..ed660927b628 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -5315,10 +5315,15 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) >> >> xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); >> xdp_prepare_buff(&ctx.xdp, page_address(buf->page), >> - buf->page_offset, buf1_len, false); >> + buf->page_offset, buf1_len, true); >> >> pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - >> buf->page_offset; >> + >> + ctx.priv = priv; >> + ctx.p = p; >> + ctx.np = np; >> + >> skb = stmmac_xdp_run_prog(priv, &ctx.xdp); >> /* Due xdp_adjust_tail: DMA sync for_device >> * cover max len CPU touch >> @@ -7071,6 +7076,23 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) >> } >> } >> >> +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 *timestamp) >> +{ >> + const struct stmmac_xdp_buff *ctx = (void *)_ctx; >> + >> + *timestamp = 0; >> + stmmac_get_rx_hwtstamp(ctx->priv, ctx->p, ctx->np, timestamp); >> + > > [..] > >> + if (*timestamp) > > Nit: does it make sense to change stmmac_get_rx_hwtstamp to return bool > to indicate success/failure? Then you can do: > > if (!stmmac_get_rx_hwtstamp()) > reutrn -ENODATA; I would make it return the -ENODATA directly since typically bool true/false functions have names like "stmmac_has_rx_hwtstamp" or similar name that infers you're answering a true/false question. That might also let you avoid zeroing the timestamp value first? Thanks, Jake
On Wed, Apr 12, 2023 at 1:56 PM Jacob Keller <jacob.e.keller@intel.com> wrote: > > > > On 4/12/2023 10:00 AM, Stanislav Fomichev wrote: > > On 04/12, Song Yoong Siang wrote: > >> Add receive hardware timestamp metadata support via kfunc to XDP receive > >> packets. > >> > >> Suggested-by: Stanislav Fomichev <sdf@google.com> > >> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> > >> --- > >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ > >> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 ++++++++++++++++++- > >> 2 files changed, 28 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > >> index ac8ccf851708..826ac0ec88c6 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > >> @@ -94,6 +94,9 @@ struct stmmac_rx_buffer { > >> > >> struct stmmac_xdp_buff { > >> struct xdp_buff xdp; > >> + struct stmmac_priv *priv; > >> + struct dma_desc *p; > >> + struct dma_desc *np; > >> }; > >> > >> struct stmmac_rx_queue { > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> index f7bbdf04d20c..ed660927b628 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > >> @@ -5315,10 +5315,15 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) > >> > >> xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); > >> xdp_prepare_buff(&ctx.xdp, page_address(buf->page), > >> - buf->page_offset, buf1_len, false); > >> + buf->page_offset, buf1_len, true); > >> > >> pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - > >> buf->page_offset; > >> + > >> + ctx.priv = priv; > >> + ctx.p = p; > >> + ctx.np = np; > >> + > >> skb = stmmac_xdp_run_prog(priv, &ctx.xdp); > >> /* Due xdp_adjust_tail: DMA sync for_device > >> * cover max len CPU touch > >> @@ -7071,6 +7076,23 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) > >> } > >> } > >> > >> +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 *timestamp) > >> +{ > >> + const struct stmmac_xdp_buff *ctx = (void *)_ctx; > >> + > >> + *timestamp = 0; > >> + stmmac_get_rx_hwtstamp(ctx->priv, ctx->p, ctx->np, timestamp); > >> + > > > > [..] > > > >> + if (*timestamp) > > > > Nit: does it make sense to change stmmac_get_rx_hwtstamp to return bool > > to indicate success/failure? Then you can do: > > > > if (!stmmac_get_rx_hwtstamp()) > > reutrn -ENODATA; > > I would make it return the -ENODATA directly since typically bool > true/false functions have names like "stmmac_has_rx_hwtstamp" or similar > name that infers you're answering a true/false question. > > That might also let you avoid zeroing the timestamp value first? SGTM! > Thanks, > Jake
On Thursday, April 13, 2023 5:46 AM, Stanislav Fomichev <sdf@google.com> wrote: >On Wed, Apr 12, 2023 at 1:56 PM Jacob Keller <jacob.e.keller@intel.com> wrote: >> >> >> >> On 4/12/2023 10:00 AM, Stanislav Fomichev wrote: >> > On 04/12, Song Yoong Siang wrote: >> >> Add receive hardware timestamp metadata support via kfunc to XDP >> >> receive packets. >> >> >> >> Suggested-by: Stanislav Fomichev <sdf@google.com> >> >> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> >> >> --- >> >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ >> >> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 >> >> ++++++++++++++++++- >> >> 2 files changed, 28 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> >> b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> >> index ac8ccf851708..826ac0ec88c6 100644 >> >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> >> @@ -94,6 +94,9 @@ struct stmmac_rx_buffer { >> >> >> >> struct stmmac_xdp_buff { >> >> struct xdp_buff xdp; >> >> + struct stmmac_priv *priv; >> >> + struct dma_desc *p; >> >> + struct dma_desc *np; >> >> }; >> >> >> >> struct stmmac_rx_queue { >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> >> index f7bbdf04d20c..ed660927b628 100644 >> >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> >> @@ -5315,10 +5315,15 @@ static int stmmac_rx(struct stmmac_priv >> >> *priv, int limit, u32 queue) >> >> >> >> xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); >> >> xdp_prepare_buff(&ctx.xdp, page_address(buf->page), >> >> - buf->page_offset, buf1_len, false); >> >> + buf->page_offset, buf1_len, >> >> + true); >> >> >> >> pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - >> >> buf->page_offset; >> >> + >> >> + ctx.priv = priv; >> >> + ctx.p = p; >> >> + ctx.np = np; >> >> + >> >> skb = stmmac_xdp_run_prog(priv, &ctx.xdp); >> >> /* Due xdp_adjust_tail: DMA sync for_device >> >> * cover max len CPU touch @@ -7071,6 +7076,23 >> >> @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) >> >> } >> >> } >> >> >> >> +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 >> >> +*timestamp) { >> >> + const struct stmmac_xdp_buff *ctx = (void *)_ctx; >> >> + >> >> + *timestamp = 0; >> >> + stmmac_get_rx_hwtstamp(ctx->priv, ctx->p, ctx->np, timestamp); >> >> + >> > >> > [..] >> > >> >> + if (*timestamp) >> > >> > Nit: does it make sense to change stmmac_get_rx_hwtstamp to return >> > bool to indicate success/failure? Then you can do: >> > >> > if (!stmmac_get_rx_hwtstamp()) >> > reutrn -ENODATA; >> >> I would make it return the -ENODATA directly since typically bool >> true/false functions have names like "stmmac_has_rx_hwtstamp" or >> similar name that infers you're answering a true/false question. >> >> That might also let you avoid zeroing the timestamp value first? > >SGTM! stmmac_get_rx_hwtstamp() is used in other places where return value is not needed. Additional if statement checking on return value will add cost, but ignoring return value will hit "unused result" warning. I think it will be more make sense if I directly retrieve the timestamp value in stmmac_xdp_rx_timestamp(), instead of reuse stmmac_get_rx_hwtstamp(). Let me send out v4 for review. Thanks & Regards Siang > >> Thanks, >> Jake
On 4/12/2023 6:39 PM, Song, Yoong Siang wrote: > On Thursday, April 13, 2023 5:46 AM, Stanislav Fomichev <sdf@google.com> wrote: >> On Wed, Apr 12, 2023 at 1:56 PM Jacob Keller <jacob.e.keller@intel.com> wrote: >>> >>> >>> >>> On 4/12/2023 10:00 AM, Stanislav Fomichev wrote: >>>> On 04/12, Song Yoong Siang wrote: >>>>> Add receive hardware timestamp metadata support via kfunc to XDP >>>>> receive packets. >>>>> >>>>> Suggested-by: Stanislav Fomichev <sdf@google.com> >>>>> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> >>>>> --- >>>>> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ >>>>> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 >>>>> ++++++++++++++++++- >>>>> 2 files changed, 28 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>>> index ac8ccf851708..826ac0ec88c6 100644 >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>>> @@ -94,6 +94,9 @@ struct stmmac_rx_buffer { >>>>> >>>>> struct stmmac_xdp_buff { >>>>> struct xdp_buff xdp; >>>>> + struct stmmac_priv *priv; >>>>> + struct dma_desc *p; >>>>> + struct dma_desc *np; >>>>> }; >>>>> >>>>> struct stmmac_rx_queue { >>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>> index f7bbdf04d20c..ed660927b628 100644 >>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>> @@ -5315,10 +5315,15 @@ static int stmmac_rx(struct stmmac_priv >>>>> *priv, int limit, u32 queue) >>>>> >>>>> xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); >>>>> xdp_prepare_buff(&ctx.xdp, page_address(buf->page), >>>>> - buf->page_offset, buf1_len, false); >>>>> + buf->page_offset, buf1_len, >>>>> + true); >>>>> >>>>> pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - >>>>> buf->page_offset; >>>>> + >>>>> + ctx.priv = priv; >>>>> + ctx.p = p; >>>>> + ctx.np = np; >>>>> + >>>>> skb = stmmac_xdp_run_prog(priv, &ctx.xdp); >>>>> /* Due xdp_adjust_tail: DMA sync for_device >>>>> * cover max len CPU touch @@ -7071,6 +7076,23 >>>>> @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) >>>>> } >>>>> } >>>>> >>>>> +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 >>>>> +*timestamp) { >>>>> + const struct stmmac_xdp_buff *ctx = (void *)_ctx; >>>>> + >>>>> + *timestamp = 0; >>>>> + stmmac_get_rx_hwtstamp(ctx->priv, ctx->p, ctx->np, timestamp); >>>>> + >>>> >>>> [..] >>>> >>>>> + if (*timestamp) >>>> >>>> Nit: does it make sense to change stmmac_get_rx_hwtstamp to return >>>> bool to indicate success/failure? Then you can do: >>>> >>>> if (!stmmac_get_rx_hwtstamp()) >>>> reutrn -ENODATA; >>> >>> I would make it return the -ENODATA directly since typically bool >>> true/false functions have names like "stmmac_has_rx_hwtstamp" or >>> similar name that infers you're answering a true/false question. >>> >>> That might also let you avoid zeroing the timestamp value first? >> >> SGTM! > > stmmac_get_rx_hwtstamp() is used in other places where return > value is not needed. Additional if statement checking on return value > will add cost, but ignoring return value will hit "unused result" warning. > Isn't unused return values only checked if the function is annotated as "__must_check"? > I think it will be more make sense if I directly retrieve the timestamp value > in stmmac_xdp_rx_timestamp(), instead of reuse stmmac_get_rx_hwtstamp(). > That makes sense too, the XDP flow is a bit special cased relative to the other ones. > Let me send out v4 for review. > > Thanks & Regards > Siang > >> >>> Thanks, >>> Jake
On Friday, April 14, 2023 12:47 AM, Keller, Jacob E <jacob.e.keller@intel.com> wrote: >On 4/12/2023 6:39 PM, Song, Yoong Siang wrote: >> On Thursday, April 13, 2023 5:46 AM, Stanislav Fomichev <sdf@google.com> >wrote: >>> On Wed, Apr 12, 2023 at 1:56 PM Jacob Keller <jacob.e.keller@intel.com> >wrote: >>>> >>>> >>>> >>>> On 4/12/2023 10:00 AM, Stanislav Fomichev wrote: >>>>> On 04/12, Song Yoong Siang wrote: >>>>>> Add receive hardware timestamp metadata support via kfunc to XDP >>>>>> receive packets. >>>>>> >>>>>> Suggested-by: Stanislav Fomichev <sdf@google.com> >>>>>> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> >>>>>> --- >>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++ >>>>>> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 >>>>>> ++++++++++++++++++- >>>>>> 2 files changed, 28 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>>>> index ac8ccf851708..826ac0ec88c6 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >>>>>> @@ -94,6 +94,9 @@ struct stmmac_rx_buffer { >>>>>> >>>>>> struct stmmac_xdp_buff { >>>>>> struct xdp_buff xdp; >>>>>> + struct stmmac_priv *priv; >>>>>> + struct dma_desc *p; >>>>>> + struct dma_desc *np; >>>>>> }; >>>>>> >>>>>> struct stmmac_rx_queue { >>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>> index f7bbdf04d20c..ed660927b628 100644 >>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>>>>> @@ -5315,10 +5315,15 @@ static int stmmac_rx(struct stmmac_priv >>>>>> *priv, int limit, u32 queue) >>>>>> >>>>>> xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); >>>>>> xdp_prepare_buff(&ctx.xdp, page_address(buf->page), >>>>>> - buf->page_offset, buf1_len, false); >>>>>> + buf->page_offset, buf1_len, >>>>>> + true); >>>>>> >>>>>> pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - >>>>>> buf->page_offset; >>>>>> + >>>>>> + ctx.priv = priv; >>>>>> + ctx.p = p; >>>>>> + ctx.np = np; >>>>>> + >>>>>> skb = stmmac_xdp_run_prog(priv, &ctx.xdp); >>>>>> /* Due xdp_adjust_tail: DMA sync for_device >>>>>> * cover max len CPU touch @@ -7071,6 >>>>>> +7076,23 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, >bool enable) >>>>>> } >>>>>> } >>>>>> >>>>>> +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 >>>>>> +*timestamp) { >>>>>> + const struct stmmac_xdp_buff *ctx = (void *)_ctx; >>>>>> + >>>>>> + *timestamp = 0; >>>>>> + stmmac_get_rx_hwtstamp(ctx->priv, ctx->p, ctx->np, >>>>>> + timestamp); >>>>>> + >>>>> >>>>> [..] >>>>> >>>>>> + if (*timestamp) >>>>> >>>>> Nit: does it make sense to change stmmac_get_rx_hwtstamp to return >>>>> bool to indicate success/failure? Then you can do: >>>>> >>>>> if (!stmmac_get_rx_hwtstamp()) >>>>> reutrn -ENODATA; >>>> >>>> I would make it return the -ENODATA directly since typically bool >>>> true/false functions have names like "stmmac_has_rx_hwtstamp" or >>>> similar name that infers you're answering a true/false question. >>>> >>>> That might also let you avoid zeroing the timestamp value first? >>> >>> SGTM! >> >> stmmac_get_rx_hwtstamp() is used in other places where return value is >> not needed. Additional if statement checking on return value will add >> cost, but ignoring return value will hit "unused result" warning. >> > >Isn't unused return values only checked if the function is annotated as >"__must_check"? I see. Dint aware that. Thanks for your info. > >> I think it will be more make sense if I directly retrieve the >> timestamp value in stmmac_xdp_rx_timestamp(), instead of reuse >stmmac_get_rx_hwtstamp(). >> > >That makes sense too, the XDP flow is a bit special cased relative to the other >ones. Yes, agree. > >> Let me send out v4 for review. >> >> Thanks & Regards >> Siang >> >>> >>>> Thanks, >>>> Jake
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index ac8ccf851708..826ac0ec88c6 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -94,6 +94,9 @@ struct stmmac_rx_buffer { struct stmmac_xdp_buff { struct xdp_buff xdp; + struct stmmac_priv *priv; + struct dma_desc *p; + struct dma_desc *np; }; struct stmmac_rx_queue { diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index f7bbdf04d20c..ed660927b628 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -5315,10 +5315,15 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) xdp_init_buff(&ctx.xdp, buf_sz, &rx_q->xdp_rxq); xdp_prepare_buff(&ctx.xdp, page_address(buf->page), - buf->page_offset, buf1_len, false); + buf->page_offset, buf1_len, true); pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start - buf->page_offset; + + ctx.priv = priv; + ctx.p = p; + ctx.np = np; + skb = stmmac_xdp_run_prog(priv, &ctx.xdp); /* Due xdp_adjust_tail: DMA sync for_device * cover max len CPU touch @@ -7071,6 +7076,23 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable) } } +static int stmmac_xdp_rx_timestamp(const struct xdp_md *_ctx, u64 *timestamp) +{ + const struct stmmac_xdp_buff *ctx = (void *)_ctx; + + *timestamp = 0; + stmmac_get_rx_hwtstamp(ctx->priv, ctx->p, ctx->np, timestamp); + + if (*timestamp) + return 0; + + return -ENODATA; +} + +static const struct xdp_metadata_ops stmmac_xdp_metadata_ops = { + .xmo_rx_timestamp = stmmac_xdp_rx_timestamp, +}; + /** * stmmac_dvr_probe * @device: device pointer @@ -7178,6 +7200,8 @@ int stmmac_dvr_probe(struct device *device, ndev->netdev_ops = &stmmac_netdev_ops; + ndev->xdp_metadata_ops = &stmmac_xdp_metadata_ops; + ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM; ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |