[xdp-hints] [PATCH net-next v3 0/4] XDP Rx HWTS metadata for stmmac driver

[xdp-hints] [PATCH net-next v3 0/4] XDP Rx HWTS metadata for stmmac driver

[Song Yoong Siang]

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

[xdp-hints] [PATCH net-next v3 1/4] net: stmmac: restructure Rx hardware timestamping function

[Song Yoong Siang]

From: Ong Boon Leong <boon.leong.ong@intel.com>

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 <boon.leong.ong@intel.com>
Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
 .../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

[xdp-hints] [PATCH net-next v3 2/4] net: stmmac: introduce wrapper for struct xdp_buff

[Song Yoong Siang]

Introduce struct stmmac_xdp_buff as a preparation to support XDP Rx
metadata via kfuncs.

Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
 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

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

[Song Yoong Siang]

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.1

[xdp-hints] [PATCH net-next v3 4/4] net: stmmac: add Rx HWTS metadata to XDP ZC receive pkt

[Song Yoong Siang]

Add receive hardware timestamp metadata support via kfunc to XDP Zero Copy
receive packets.

Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
---
 .../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

[xdp-hints] Re: [PATCH net-next v3 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Stanislav Fomichev]

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;

[xdp-hints] Re: [PATCH net-next v3 1/4] net: stmmac: restructure Rx hardware timestamping function

[Jacob Keller]

On 4/12/2023 2:42 AM, Song Yoong Siang wrote:
> From: Ong Boon Leong <boon.leong.ong@intel.com>
> 
> 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 <boon.leong.ong@intel.com>
> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

[xdp-hints] Re: [PATCH net-next v3 2/4] net: stmmac: introduce wrapper for struct xdp_buff

[Jacob Keller]

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 <yoong.siang.song@intel.com>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

[xdp-hints] Re: [PATCH net-next v3 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Jacob Keller]

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

[xdp-hints] Re: [PATCH net-next v3 4/4] net: stmmac: add Rx HWTS metadata to XDP ZC receive pkt

[Jacob Keller]

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 <yoong.siang.song@intel.com>
> ---

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.

[xdp-hints] Re: [PATCH net-next v3 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Stanislav Fomichev]

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

[xdp-hints] Re: [PATCH net-next v3 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Song, Yoong Siang]

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

[xdp-hints] Re: [PATCH net-next v3 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Jacob Keller]

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

[xdp-hints] Re: [PATCH net-next v3 3/4] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Song, Yoong Siang]

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