RE: [PATCH net-next v5 2/3] net: stmmac: add Rx HWTS metadata to XDP receive pkt

[Song, Yoong Siang]

On Saturday, April 15, 2023 12:39 AM, Jesper Dangaard Brouer <jbrouer@xxxxxxxxxx> wrote:
>On 14/04/2023 07.26, Song Yoong Siang wrote:
>> Add receive hardware timestamp metadata support via kfunc to XDP
>> receive packets.
>>
>> Suggested-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
>> Signed-off-by: Song Yoong Siang <yoong.siang.song@xxxxxxxxx>
>> Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  3 ++
>>   .../net/ethernet/stmicro/stmmac/stmmac_main.c | 40 ++++++++++++++++++-
>>   2 files changed, 42 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;
>
>Hmm, I don't like the naming of the descriptors as "p" and "np".
>If you insist on this naming, at least we need comments describing that this is.
>
>Does "desc" and "ndesc" make sense?  (where "n" means "next")
>

Yup, make sense to have desc and ndesc. I will update the naming in V6.

Thanks & Regards
Siang

>>   };
>>
>>   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 10b9f8912bb2..74f78e5537a3 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -5313,10 +5313,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
>> @@ -7060,6 +7065,37 @@ 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;
>> +	struct stmmac_priv *priv = ctx->priv;
>> +	struct dma_desc *desc = ctx->p;
>> +	struct dma_desc *np = ctx->np;
>> +	struct dma_desc *p = ctx->p;
>> +	u64 ns = 0;
>> +
>> +	if (!priv->hwts_rx_en)
>> +		return -ENODATA;
>> +
>> +	/* For GMAC4, the valid timestamp is from CTX next desc. */
>> +	if (priv->plat->has_gmac4 || priv->plat->has_xgmac)
>> +		desc = np;
>> +
>> +	/* Check if timestamp is available */
>> +	if (stmmac_get_rx_timestamp_status(priv, p, np, priv->adv_ts)) {
>> +		stmmac_get_timestamp(priv, desc, priv->adv_ts, &ns);
>> +		ns -= priv->plat->cdc_error_adj;
>> +		*timestamp = ns_to_ktime(ns);
>> +		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
>> @@ -7167,6 +7203,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 |