Implemented XDP receive hardware timestamp metadata for stmmac driver.
This patchset is tested with tools/testing/selftests/bpf/xdp_hw_metadata.
Below are the test steps and results.
Command on DUT:
sudo ./xdp_hw_metadata <interface name>
Command on Link Partner:
echo -n xdp | nc -u -q1 <destination IPv4 addr> 9091
echo -n skb | nc -u -q1 <destination IPv4 addr> 9092
Result for port 9091:
0x55fdb5f006d0: rx_desc[3]->addr=1000000003bd000 addr=3bd100 comp_addr=3bd000
rx_timestamp: 1677762474360150047
rx_hash: 0
0x55fdb5f006d0: complete idx=515 addr=3bd000
Result for port 9092:
found skb hwtstamp = 1677762476.320146161
Changes since v2:
* To reduce packet processing cost, get the Rx HWTS only when xmo_rx_timestamp() is called
Changes since v1:
* Add static to stmmac_xdp_metadata_ops declaration
---
Ong Boon Leong (1):
net: stmmac: restructure Rx hardware timestamping function
Song Yoong Siang (3):
net: stmmac: introduce wrapper for struct xdp_buff
net: stmmac: add Rx HWTS metadata to XDP receive pkt
net: stmmac: add Rx HWTS metadata to XDP ZC receive pkt
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 7 ++
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 87 +++++++++++++++----
2 files changed, 76 insertions(+), 18 deletions(-)
--
2.34.1
From: Ong Boon Leong <[email protected]>
Rearrange the function of getting Rx hardware timestamp for skb so
that it can be reused for XDP later.
Signed-off-by: Ong Boon Leong <[email protected]>
Signed-off-by: Song Yoong Siang <[email protected]>
---
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 ++++++++++++-------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 04fbb7770618..2cc6237a9c28 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -570,15 +570,14 @@ static void stmmac_get_tx_hwtstamp(struct stmmac_priv *priv,
* @priv: driver private structure
* @p : descriptor pointer
* @np : next descriptor pointer
- * @skb : the socket buffer
+ * @hwtstamp : hardware timestamp
* Description :
* This function will read received packet's timestamp from the descriptor
* and pass it to stack. It also perform some sanity checks.
*/
static void stmmac_get_rx_hwtstamp(struct stmmac_priv *priv, struct dma_desc *p,
- struct dma_desc *np, struct sk_buff *skb)
+ struct dma_desc *np, ktime_t *hwtstamp)
{
- struct skb_shared_hwtstamps *shhwtstamp = NULL;
struct dma_desc *desc = p;
u64 ns = 0;
@@ -595,9 +594,7 @@ static void stmmac_get_rx_hwtstamp(struct stmmac_priv *priv, struct dma_desc *p,
ns -= priv->plat->cdc_error_adj;
netdev_dbg(priv->dev, "get valid RX hw timestamp %llu\n", ns);
- shhwtstamp = skb_hwtstamps(skb);
- memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps));
- shhwtstamp->hwtstamp = ns_to_ktime(ns);
+ *hwtstamp = ns_to_ktime(ns);
} else {
netdev_dbg(priv->dev, "cannot get RX hw timestamp\n");
}
@@ -4909,6 +4906,7 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
struct xdp_buff *xdp)
{
struct stmmac_channel *ch = &priv->channel[queue];
+ struct skb_shared_hwtstamps *shhwtstamp = NULL;
unsigned int len = xdp->data_end - xdp->data;
enum pkt_hash_types hash_type;
int coe = priv->hw->rx_csum;
@@ -4921,7 +4919,10 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
return;
}
- stmmac_get_rx_hwtstamp(priv, p, np, skb);
+ shhwtstamp = skb_hwtstamps(skb);
+ memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps));
+ stmmac_get_rx_hwtstamp(priv, p, np, &shhwtstamp->hwtstamp);
+
stmmac_rx_vlan(priv->dev, skb);
skb->protocol = eth_type_trans(skb, priv->dev);
@@ -5213,6 +5214,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
rx_q->dma_rx_phy, desc_size);
}
while (count < limit) {
+ struct skb_shared_hwtstamps *shhwtstamp = NULL;
unsigned int buf1_len = 0, buf2_len = 0;
enum pkt_hash_types hash_type;
struct stmmac_rx_buffer *buf;
@@ -5407,7 +5409,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
/* Got entire packet into SKB. Finish it. */
- stmmac_get_rx_hwtstamp(priv, p, np, skb);
+ shhwtstamp = skb_hwtstamps(skb);
+ memset(shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps));
+ stmmac_get_rx_hwtstamp(priv, p, np, &shhwtstamp->hwtstamp);
+
stmmac_rx_vlan(priv->dev, skb);
skb->protocol = eth_type_trans(skb, priv->dev);
--
2.34.1
Introduce struct stmmac_xdp_buff as a preparation to support XDP Rx
metadata via kfuncs.
Signed-off-by: Song Yoong Siang <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 4 ++++
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 18 +++++++++---------
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 3d15e1e92e18..ac8ccf851708 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -92,6 +92,10 @@ struct stmmac_rx_buffer {
dma_addr_t sec_addr;
};
+struct stmmac_xdp_buff {
+ struct xdp_buff xdp;
+};
+
struct stmmac_rx_queue {
u32 rx_count_frames;
u32 queue_index;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 2cc6237a9c28..f7bbdf04d20c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5189,9 +5189,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
int status = 0, coe = priv->hw->rx_csum;
unsigned int next_entry = rx_q->cur_rx;
enum dma_data_direction dma_dir;
+ struct stmmac_xdp_buff ctx = {};
unsigned int desc_size;
struct sk_buff *skb = NULL;
- struct xdp_buff xdp;
int xdp_status = 0;
int buf_sz;
@@ -5313,17 +5313,17 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
dma_sync_single_for_cpu(priv->device, buf->addr,
buf1_len, dma_dir);
- xdp_init_buff(&xdp, buf_sz, &rx_q->xdp_rxq);
- xdp_prepare_buff(&xdp, page_address(buf->page),
+ 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);
- pre_len = xdp.data_end - xdp.data_hard_start -
+ pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
buf->page_offset;
- skb = stmmac_xdp_run_prog(priv, &xdp);
+ skb = stmmac_xdp_run_prog(priv, &ctx.xdp);
/* Due xdp_adjust_tail: DMA sync for_device
* cover max len CPU touch
*/
- sync_len = xdp.data_end - xdp.data_hard_start -
+ sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
buf->page_offset;
sync_len = max(sync_len, pre_len);
@@ -5333,7 +5333,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
if (xdp_res & STMMAC_XDP_CONSUMED) {
page_pool_put_page(rx_q->page_pool,
- virt_to_head_page(xdp.data),
+ virt_to_head_page(ctx.xdp.data),
sync_len, true);
buf->page = NULL;
priv->dev->stats.rx_dropped++;
@@ -5361,7 +5361,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
if (!skb) {
/* XDP program may expand or reduce tail */
- buf1_len = xdp.data_end - xdp.data;
+ buf1_len = ctx.xdp.data_end - ctx.xdp.data;
skb = napi_alloc_skb(&ch->rx_napi, buf1_len);
if (!skb) {
@@ -5371,7 +5371,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
}
/* XDP program may adjust header */
- skb_copy_to_linear_data(skb, xdp.data, buf1_len);
+ skb_copy_to_linear_data(skb, ctx.xdp.data, buf1_len);
skb_put(skb, buf1_len);
/* Data payload copied into SKB, page ready for recycle */
--
2.34.1
Add receive hardware timestamp metadata support via kfunc to XDP Zero Copy
receive packets.
Signed-off-by: Song Yoong Siang <[email protected]>
---
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 22 +++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ed660927b628..1c4dbac90717 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1611,6 +1611,12 @@ static int stmmac_alloc_rx_buffers_zc(struct stmmac_priv *priv,
struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
int i;
+ /* struct stmmac_xdp_buff is using cb field (maximum size of 24 bytes)
+ * in struct xdp_buff_xsk to stash driver specific information. Thus,
+ * use this macro to make sure no size violations.
+ */
+ XSK_CHECK_PRIV_TYPE(struct stmmac_xdp_buff);
+
for (i = 0; i < dma_conf->dma_rx_size; i++) {
struct stmmac_rx_buffer *buf;
dma_addr_t dma_addr;
@@ -4999,6 +5005,16 @@ static bool stmmac_rx_refill_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
return ret;
}
+static struct stmmac_xdp_buff *xsk_buff_to_stmmac_ctx(struct xdp_buff *xdp)
+{
+ /* In XDP zero copy data path, xdp field in struct xdp_buff_xsk is used
+ * to represent incoming packet, whereas cb field in the same structure
+ * is used to store driver specific info. Thus, struct stmmac_xdp_buff
+ * is laid on top of xdp and cb fields of struct xdp_buff_xsk.
+ */
+ return (struct stmmac_xdp_buff *)xdp;
+}
+
static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
{
struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
@@ -5028,6 +5044,7 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
}
while (count < limit) {
struct stmmac_rx_buffer *buf;
+ struct stmmac_xdp_buff *ctx;
unsigned int buf1_len = 0;
struct dma_desc *np, *p;
int entry;
@@ -5113,6 +5130,11 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
goto read_again;
}
+ ctx = xsk_buff_to_stmmac_ctx(buf->xdp);
+ ctx->priv = priv;
+ ctx->p = p;
+ ctx->np = np;
+
/* XDP ZC Frame only support primary buffers for now */
buf1_len = stmmac_rx_buf1_len(priv, p, status, len);
len += buf1_len;
--
2.34.1
Add receive hardware timestamp metadata support via kfunc to XDP receive
packets.
Suggested-by: Stanislav Fomichev <[email protected]>
Signed-off-by: Song Yoong Siang <[email protected]>
---
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.1
On 04/12, Song Yoong Siang wrote:
> Add receive hardware timestamp metadata support via kfunc to XDP receive
> packets.
>
> Suggested-by: Stanislav Fomichev <[email protected]>
> Signed-off-by: Song Yoong Siang <[email protected]>
> ---
> 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 2:42 AM, Song Yoong Siang wrote:
> From: Ong Boon Leong <[email protected]>
>
> Rearrange the function of getting Rx hardware timestamp for skb so
> that it can be reused for XDP later.
>
> Signed-off-by: Ong Boon Leong <[email protected]>
> Signed-off-by: Song Yoong Siang <[email protected]>
> ---
Reviewed-by: Jacob Keller <[email protected]>
On 4/12/2023 2:42 AM, Song Yoong Siang wrote:
> Introduce struct stmmac_xdp_buff as a preparation to support XDP Rx
> metadata via kfuncs.
>
> Signed-off-by: Song Yoong Siang <[email protected]>
> ---
Reviewed-by: Jacob Keller <[email protected]>
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 <[email protected]>
>> Signed-off-by: Song Yoong Siang <[email protected]>
>> ---
>> 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 4/12/2023 2:42 AM, Song Yoong Siang wrote:
> Add receive hardware timestamp metadata support via kfunc to XDP Zero Copy
> receive packets.
>
> Signed-off-by: Song Yoong Siang <[email protected]>
> ---
I'm not familiar enough with XDP to understand how the struct layering
and casting works, so I'm not confident in adding my Reviewed-by, but
this seems ok to me.
On Wed, Apr 12, 2023 at 1:56 PM Jacob Keller <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Song Yoong Siang <[email protected]>
> >> ---
> >> 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 <[email protected]> wrote:
>On Wed, Apr 12, 2023 at 1:56 PM Jacob Keller <[email protected]> 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 <[email protected]>
>> >> Signed-off-by: Song Yoong Siang <[email protected]>
>> >> ---
>> >> 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 <[email protected]> wrote:
>> On Wed, Apr 12, 2023 at 1:56 PM Jacob Keller <[email protected]> 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 <[email protected]>
>>>>> Signed-off-by: Song Yoong Siang <[email protected]>
>>>>> ---
>>>>> 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 <[email protected]> wrote:
>On 4/12/2023 6:39 PM, Song, Yoong Siang wrote:
>> On Thursday, April 13, 2023 5:46 AM, Stanislav Fomichev <[email protected]>
>wrote:
>>> On Wed, Apr 12, 2023 at 1:56 PM Jacob Keller <[email protected]>
>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 <[email protected]>
>>>>>> Signed-off-by: Song Yoong Siang <[email protected]>
>>>>>> ---
>>>>>> 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