Re: [Intel-wired-lan] [PATCH net] igc: Fix RX HW timestamp reporting when NET_RX_BUSY_POLL is disabled

[Paul Menzel]

Dear Ding,


Thank you for your patch.

Am 22.06.26 um 06:13 schrieb Ding Meng via Intel-wired-lan:
> When CONFIG_NET_RX_BUSY_POLL is deactivated, fetching RX HW timestamps
> from the NIC no longer works as expected.

Maybe paste some logs/errors, so it can be easier found by people with 
the same issue.

> This occurs because disabling CONFIG_NET_RX_BUSY_POLL disables the
> SKB NAPI mapping in __skb_mark_napi_id(). Consequently, get_timestamp()
> fails to perform its driver lookup, and the igc driver's struct
> net_device_ops::ndo_get_tstamp is never invoked.
>
> Instead, get_timestamp() falls back to use shhwtstamps(skb)->hwtstamp,
> a field that the driver has not populated.
>
> Fix this by populating the hwtstamp field with the correct timestamp
> in the default timer when CONFIG_NET_RX_BUSY_POLL is disabled.

Maybe detail, why the adapter needs to be passed now.

Also, please describe a test case to check the change.

> Fixes: 069b142f5819 ("igc: Add support for PTP .getcyclesx64()")
> Co-developed-by: Florian Bezdeka <florian.bezdeka@xxxxxxxxxxx>
> Signed-off-by: Florian Bezdeka <florian.bezdeka@xxxxxxxxxxx>
> Signed-off-by: Ding Meng <meng.ding@xxxxxxxxxxx>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 38 ++++++++++++++++-------
>   1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 8ac16808023..1da8d7aa76d 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1992,7 +1992,26 @@ static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring,
>   	return skb;
>   }
>   
> -static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
> +static void igc_construct_skb_timestamps(struct igc_adapter *adapter,
> +					 struct sk_buff *skb,
> +					 struct igc_xdp_buff *ctx)
> +{
> +	if (!ctx->rx_ts)
> +		return;
> +#ifdef CONFIG_NET_RX_BUSY_POLL

Is there a way to do this in C instead of the pre-processor. That way 
all the code gets build tested. (Is there a config with disabled 
NET_RX_BUSY_POLL?)

> +	skb_shinfo(skb)->tx_flags |= SKBTX_HW_TSTAMP_NETDEV;
> +	skb_hwtstamps(skb)->netdev_data = ctx->rx_ts;
> +#else
> +	struct igc_inline_rx_tstamps *tstamps;
> +
> +	tstamps = ctx->rx_ts;
> +	skb_hwtstamps(skb)->hwtstamp = igc_ptp_rx_pktstamp(adapter,
> +							   tstamps->timer0);
> +#endif
> +}
> +
> +static struct sk_buff *igc_construct_skb(struct igc_adapter *adapter,
> +					 struct igc_ring *rx_ring,
>   					 struct igc_rx_buffer *rx_buffer,
>   					 struct igc_xdp_buff *ctx)
>   {
> @@ -2013,10 +2032,7 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
>   	if (unlikely(!skb))
>   		return NULL;
>   
> -	if (ctx->rx_ts) {
> -		skb_shinfo(skb)->tx_flags |= SKBTX_HW_TSTAMP_NETDEV;
> -		skb_hwtstamps(skb)->netdev_data = ctx->rx_ts;
> -	}
> +	igc_construct_skb_timestamps(adapter, skb, ctx);
>   
>   	/* Determine available headroom for copy */
>   	headlen = size;
> @@ -2686,7 +2702,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>   		else if (ring_uses_build_skb(rx_ring))
>   			skb = igc_build_skb(rx_ring, rx_buffer, &ctx.xdp);
>   		else
> -			skb = igc_construct_skb(rx_ring, rx_buffer, &ctx);
> +			skb = igc_construct_skb(adapter, rx_ring, rx_buffer, &ctx);
>   
>   		/* exit if we failed to retrieve a buffer */
>   		if (!xdp_res && !skb) {
> @@ -2738,7 +2754,8 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>   	return total_packets;
>   }
>   
> -static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring,
> +static struct sk_buff *igc_construct_skb_zc(struct igc_adapter *adapter,
> +					    struct igc_ring *ring,
>   					    struct igc_xdp_buff *ctx)
>   {
>   	struct xdp_buff *xdp = &ctx->xdp;
> @@ -2760,10 +2777,7 @@ static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring,
>   		__skb_pull(skb, metasize);
>   	}
>   
> -	if (ctx->rx_ts) {
> -		skb_shinfo(skb)->tx_flags |= SKBTX_HW_TSTAMP_NETDEV;
> -		skb_hwtstamps(skb)->netdev_data = ctx->rx_ts;
> -	}
> +	igc_construct_skb_timestamps(adapter, skb, ctx);
>   
>   	return skb;
>   }
> @@ -2775,7 +2789,7 @@ static void igc_dispatch_skb_zc(struct igc_q_vector *q_vector,
>   	struct igc_ring *ring = q_vector->rx.ring;
>   	struct sk_buff *skb;
>   
> -	skb = igc_construct_skb_zc(ring, ctx);
> +	skb = igc_construct_skb_zc(q_vector->adapter, ring, ctx);
>   	if (!skb) {
>   		ring->rx_stats.alloc_failed++;
>   		set_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &ring->flags);

Otherwise this looks good.


Kind regards,

Paul