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

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> 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;
>

Good catch. Will fix.

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

That's a good point. Yeah, if noone has enabled hardware timestamps,
then there's no timestamps requests able to timeout. Will take a closer
look and see if I am missing anything, but good idea. Will fix.

>>  
>> -	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);
>>  }

-- 
Vinicius
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan