diff mbox series

[net-next,v3,3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

Message ID 20230412094235.589089-4-yoong.siang.song@intel.com
State New
Headers
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

Stanislav Fomichev April 12, 2023, 5 p.m. UTC | #1 
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;
Jacob Keller April 12, 2023, 8:55 p.m. UTC | #2 
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
Stanislav Fomichev April 12, 2023, 9:45 p.m. UTC | #3 
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
Song Yoong Siang April 13, 2023, 1:39 a.m. UTC | #4 
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
Jacob Keller April 13, 2023, 4:47 p.m. UTC | #5 
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
Song Yoong Siang April 14, 2023, 1:01 a.m. UTC | #6 
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 mbox series

Patch

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 |