| Message ID | 20230412094235.589089-4-yoong.siang.song@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | XDP Rx HWTS metadata for stmmac driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 43 this patch: 43 |
| netdev/cc_maintainers | success | CCed 16 of 16 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 43 this patch: 43 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 56 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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 |
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(-)