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)
+ 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 |
--
2.34.1On 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
© 2016 - 2026 Red Hat, Inc.