[Intel-wired-lan] [PATCH next-queue v1 2/3] igc: Add support for multiple in-flight TX timestamps

[Vladimir Oltean]

On Mon, Feb 27, 2023 at 09:45:33PM -0800, Vinicius Costa Gomes wrote:
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index e804a566bdd3..f0617838a16a 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -35,6 +35,8 @@ void igc_ethtool_set_ops(struct net_device *);
>  
>  #define MAX_FLEX_FILTER			32
>  
> +#define IGC_MAX_TX_TSTAMP_REGS		4
> +
>  enum igc_mac_filter_type {
>  	IGC_MAC_FILTER_TYPE_DST = 0,
>  	IGC_MAC_FILTER_TYPE_SRC
> @@ -67,6 +69,15 @@ struct igc_rx_packet_stats {
>  	u64 other_packets;
>  };
>  
> +struct igc_tx_timestamp_request {
> +	struct sk_buff *skb;   /* reference to the packet being timestamped */
> +	unsigned long start;   /* when the tstamp request started (jiffies) */
> +	u32 mask;              /* _TSYNCTXCTL_TXTT_{X} bit for this request */
> +	u32 regl;              /* which TXSTMPL_{X} register should be used */
> +	u32 regh;              /* which TXSTMPH_{X} register should be used */
> +	u32 flags;             /* flags that should be added to the tx_buffer */
> +};
> +
>  struct igc_ring_container {
>  	struct igc_ring *ring;          /* pointer to linked list of rings */
>  	unsigned int total_bytes;       /* total bytes processed this int */
> @@ -231,9 +242,8 @@ struct igc_adapter {
>  	 * ptp_tx_lock.
>  	 */
>  	spinlock_t ptp_tx_lock;
> -	struct sk_buff *ptp_tx_skb;
> +	struct igc_tx_timestamp_request tx_tstamp[IGC_MAX_TX_TSTAMP_REGS];
>  	struct hwtstamp_config tstamp_config;
> -	unsigned long ptp_tx_start;

This comment added by patch 1/3 already became obsolete:

	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
	 * ptp_tx_lock.
	 */
	spinlock_t ptp_tx_lock;

>  	unsigned int ptp_flags;
>  	/* System time value lock */
>  	spinlock_t tmreg_lock;
> @@ -416,6 +426,10 @@ enum igc_tx_flags {
>  	/* olinfo flags */
>  	IGC_TX_FLAGS_IPV4	= 0x10,
>  	IGC_TX_FLAGS_CSUM	= 0x20,
> +
> +	IGC_TX_FLAGS_TSTAMP_1	= 0x100,
> +	IGC_TX_FLAGS_TSTAMP_2	= 0x200,
> +	IGC_TX_FLAGS_TSTAMP_3	= 0x400,
>  };
>  
>  enum igc_boards {
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index 9247395911c9..0cb932b52a7b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -604,92 +604,109 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
>  }
>  
>  /* Requires adapter->ptp_tx_lock held by caller. */
> -static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
> +static void igc_ptp_tx_timeout(struct igc_adapter *adapter,
> +			       struct igc_tx_timestamp_request *tstamp)
>  {
>  	struct igc_hw *hw = &adapter->hw;
>  
> -	dev_kfree_skb_any(adapter->ptp_tx_skb);
> -	adapter->ptp_tx_skb = NULL;
> -	adapter->ptp_tx_start = 0;
> +	dev_kfree_skb_any(tstamp->skb);
> +	tstamp->skb = NULL;
> +	tstamp->start = 0;
>  	adapter->tx_hwtstamp_timeouts++;
> -	/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
> -	rd32(IGC_TXSTMPH);
> +
> +	/* Reading the high register of one of the timestamp registers
> +	 * clears the equivalent bit in the TSYNCTXCTL register.
> +	 */
> +	rd32(tstamp->regh);
> +
>  	netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
>  }
>  
>  void igc_ptp_tx_hang(struct igc_adapter *adapter)
>  {
> +	struct igc_tx_timestamp_request *tstamp;
>  	unsigned long flags;
> +	int i;
>  
>  	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);

Since this is called unconditionally and periodically from igc_watchdog_task(),
it will periodically temporarily disable IRQs and therefore also
softirqs, to not race with the data path, right?

Does it make sense to only do this if adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON?
There shouldn't be TX timestamps in flight if that isn't set, and an
_irqsave could be avoided.

I suppose this is more of a comment on patch 1/3 really.

>  
> -	if (!adapter->ptp_tx_skb)
> -		goto unlock;
> +	for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) {
> +		tstamp = &adapter->tx_tstamp[i];
>  
> -	if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
> -		goto unlock;
> +		if (!tstamp->skb)
> +			continue;
>  
> -	igc_ptp_tx_timeout(adapter);
> +		if (time_is_after_jiffies(tstamp->start + IGC_PTP_TX_TIMEOUT))
> +			continue;
> +
> +		igc_ptp_tx_timeout(adapter, tstamp);
> +	}
>  
> -unlock:
>  	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>  }