netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 1/1] igc: ptp: Add 1-step functionality to igc driver
@ 2022-09-08 22:18 Lasse Johnsen
  2022-09-08 22:28 ` Richard Cochran
  2022-09-09 15:53 ` kernel test robot
  0 siblings, 2 replies; 9+ messages in thread
From: Lasse Johnsen @ 2022-09-08 22:18 UTC (permalink / raw)
  To: netdev, Tony Nguyen, Jesse Brandeburg
  Cc: Stanton, Kevin B, Jonathan Lemon, Richard Cochran

This patch adds 1-step functionality to the igc driver
1-step is only supported in L2 PTP
(... as the hardware can update the FCS, but not the UDP checksum on the fly..)

Signed-off-by: Lasse Johnsen <lasse@timebeat.app>
---
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 1e7e707..70d9440 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -247,6 +247,8 @@ struct igc_adapter {
 		struct timespec64 start;
 		struct timespec64 period;
 	} perout[IGC_N_PEROUT];
+
+	bool onestep_discard;
 };
 
 void igc_up(struct igc_adapter *adapter);
@@ -403,13 +405,14 @@ enum igc_state_t {
 
 enum igc_tx_flags {
 	/* cmd_type flags */
-	IGC_TX_FLAGS_VLAN	= 0x01,
-	IGC_TX_FLAGS_TSO	= 0x02,
-	IGC_TX_FLAGS_TSTAMP	= 0x04,
+	IGC_TX_FLAGS_VLAN		= 0x01,
+	IGC_TX_FLAGS_TSO		= 0x02,
+	IGC_TX_FLAGS_TSTAMP		= 0x04,
+	IGC_TX_FLAGS_ONESTEP_SYNC	= 0x08,
 
 	/* olinfo flags */
-	IGC_TX_FLAGS_IPV4	= 0x10,
-	IGC_TX_FLAGS_CSUM	= 0x20,
+	IGC_TX_FLAGS_IPV4		= 0x10,
+	IGC_TX_FLAGS_CSUM		= 0x20,
 };
 
 enum igc_boards {
@@ -631,6 +634,7 @@ int igc_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr);
 int igc_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr);
 void igc_ptp_tx_hang(struct igc_adapter *adapter);
 void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts);
+bool igc_ptp_process_onestep(struct igc_adapter *adapter, struct sk_buff *skb);
 
 #define igc_rx_pg_size(_ring) (PAGE_SIZE << igc_rx_pg_order(_ring))
 
diff --git a/drivers/net/ethernet/intel/igc/igc_base.h b/drivers/net/ethernet/intel/igc/igc_base.h
index ce530f5..a7106b5 100644
--- a/drivers/net/ethernet/intel/igc/igc_base.h
+++ b/drivers/net/ethernet/intel/igc/igc_base.h
@@ -31,6 +31,7 @@ struct igc_adv_tx_context_desc {
 };
 
 /* Adv Transmit Descriptor Config Masks */
+#define IGC_ADVTXD_ONESTEP	0x00040000 /* IEEE1588 perform one-step */
 #define IGC_ADVTXD_MAC_TSTAMP	0x00080000 /* IEEE1588 Timestamp packet */
 #define IGC_ADVTXD_DTYP_CTXT	0x00200000 /* Advanced Context Descriptor */
 #define IGC_ADVTXD_DTYP_DATA	0x00300000 /* Advanced Data Descriptor */
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 8cc077b..cba34ae 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1536,7 +1536,8 @@ static int igc_ethtool_get_ts_info(struct net_device *dev,
 
 		info->tx_types =
 			BIT(HWTSTAMP_TX_OFF) |
-			BIT(HWTSTAMP_TX_ON);
+			BIT(HWTSTAMP_TX_ON) |
+			BIT(HWTSTAMP_TX_ONESTEP_SYNC);
 
 		info->rx_filters = BIT(HWTSTAMP_FILTER_NONE);
 		info->rx_filters |= BIT(HWTSTAMP_FILTER_ALL);
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index a5ebee7..713da04 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1260,6 +1260,9 @@ static int igc_tx_map(struct igc_ring *tx_ring,
 	cmd_type |= size | IGC_TXD_DCMD;
 	tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type);
 
+	if (first->tx_flags & IGC_TX_FLAGS_ONESTEP_SYNC)
+		tx_desc->read.cmd_type_len |= IGC_ADVTXD_ONESTEP;
+
 	netdev_tx_sent_queue(txring_txq(tx_ring), first->bytecount);
 
 	/* set the timestamp */
@@ -1452,12 +1455,19 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 		 * the other timer registers before skipping the
 		 * timestamping request.
 		 */
-		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
+
+		if (adapter->tstamp_config.tx_type >= HWTSTAMP_TX_ON &&
 		    !test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
 					   &adapter->state)) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			tx_flags |= IGC_TX_FLAGS_TSTAMP;
 
+			if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ONESTEP_SYNC &&
+			    igc_ptp_process_onestep(adapter, skb)) {
+				tx_flags |= IGC_TX_FLAGS_ONESTEP_SYNC;
+				adapter->onestep_discard = true;
+			}
+
 			adapter->ptp_tx_skb = skb_get(skb);
 			adapter->ptp_tx_start = jiffies;
 		} else {
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 653e9f1..dc57339 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -21,6 +21,9 @@
 #define IGC_PTM_STAT_SLEEP		2
 #define IGC_PTM_STAT_TIMEOUT		100
 
+#define IGC_PTP_TX_ONESTEP_L2_OFFSET		48
+#define IGC_PTP_TX_ONESTEP_L2_IN_VLAN_OFFSET	52
+
 /* SYSTIM read access for I225 */
 void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts)
 {
@@ -550,6 +553,35 @@ static void igc_ptp_enable_tx_timestamp(struct igc_adapter *adapter)
 	rd32(IGC_TXSTMPH);
 }
 
+static void igc_ptp_update_1588_offset_in_tsynctxctl(struct igc_adapter *adapter, int ptp_class)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u32 onestep_offset = (rd32(IGC_TSYNCTXCTL) & 0xFF00) >> 8;
+
+	if ((ptp_class & PTP_CLASS_VLAN) &&
+	    onestep_offset != IGC_PTP_TX_ONESTEP_L2_IN_VLAN_OFFSET) {
+		wr32(IGC_TSYNCTXCTL, IGC_TSYNCTXCTL_ENABLED | IGC_TSYNCTXCTL_TXSYNSIG |
+				(IGC_PTP_TX_ONESTEP_L2_IN_VLAN_OFFSET << 8));
+	} else if (!(ptp_class & PTP_CLASS_VLAN) &&
+		   onestep_offset != IGC_PTP_TX_ONESTEP_L2_OFFSET) {
+		wr32(IGC_TSYNCTXCTL, IGC_TSYNCTXCTL_ENABLED | IGC_TSYNCTXCTL_TXSYNSIG |
+				(IGC_PTP_TX_ONESTEP_L2_OFFSET << 8));
+	}
+}
+
+bool igc_ptp_process_onestep(struct igc_adapter *adapter, struct sk_buff *skb)
+{
+	unsigned int ptp_class = ptp_classify_raw(skb);
+
+	if ((ptp_class & PTP_CLASS_L2) && (ptp_class & PTP_CLASS_V2)) {
+		if (ptp_msg_is_sync(skb, ptp_class)) {
+			igc_ptp_update_1588_offset_in_tsynctxctl(adapter, ptp_class);
+			return true;
+		}
+	}
+	return false;
+}
+
 /**
  * igc_ptp_set_timestamp_mode - setup hardware for timestamping
  * @adapter: networking device structure
@@ -564,6 +596,7 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
 	case HWTSTAMP_TX_OFF:
 		igc_ptp_disable_tx_timestamp(adapter);
 		break;
+	case HWTSTAMP_TX_ONESTEP_SYNC:
 	case HWTSTAMP_TX_ON:
 		igc_ptp_enable_tx_timestamp(adapter);
 		break;
@@ -603,6 +636,9 @@ static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
 
+	if (adapter->onestep_discard)
+		adapter->onestep_discard = false;
+
 	dev_kfree_skb_any(adapter->ptp_tx_skb);
 	adapter->ptp_tx_skb = NULL;
 	adapter->tx_hwtstamp_timeouts++;
@@ -671,16 +707,14 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
 	shhwtstamps.hwtstamp =
 		ktime_add_ns(shhwtstamps.hwtstamp, adjust);
 
-	/* Clear the lock early before calling skb_tstamp_tx so that
-	 * applications are not woken up before the lock bit is clear. We use
-	 * a copy of the skb pointer to ensure other threads can't change it
-	 * while we're notifying the stack.
-	 */
+	if (adapter->onestep_discard)
+		adapter->onestep_discard = false;
+	else
+		skb_tstamp_tx(skb, &shhwtstamps);
+
 	adapter->ptp_tx_skb = NULL;
 	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
 
-	/* Notify the stack and free the skb after we've unlocked */
-	skb_tstamp_tx(skb, &shhwtstamps);
 	dev_kfree_skb_any(skb);
 }
 
@@ -959,6 +993,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
 
 	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
 	adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
+	adapter->onestep_discard = false;
 
 	adapter->prev_ptp_time = ktime_to_timespec64(ktime_get_real());
 	adapter->ptp_reset_start = ktime_get();
-- 
2.37.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/1] igc: ptp: Add 1-step functionality to igc driver
@ 2022-09-10 15:07 Lasse Johnsen
  2022-09-12 15:43 ` Richard Cochran
  0 siblings, 1 reply; 9+ messages in thread
From: Lasse Johnsen @ 2022-09-10 15:07 UTC (permalink / raw)
  To: Richard Cochran
  Cc: netdev, Tony Nguyen, Jesse Brandeburg, Stanton, Kevin B,
	Jonathan Lemon

Hi Richard,

I understand your point and your concern.

Would you be amenable to an addition to the API so we can take advantage of 
hardware that offers only a subset of the options?

We could for example extend granularity of the HWTSTAMP TX API to make requests 
for different features visible to the user space applications directly. So the TX side 
would become much more granular as is already the case with the RX side. We could 
add HWTSTAMP_TX_ONESTEP_SYNC_L2_V2, HWTSTAMP_TX_ONESTEP_SYNC_L4_V2 etc. 

My worry is that if we do not do this, then the ONESTEP option will continue 
to not see much use because so many permutations (L2, UDPv4, UDPv6, V1, V2, VLAN etc.)
currently have to be supported by the hardware.

I like Intel’s cards. I want to support the features provided by the hardware… :-)

If you agree with this approach, then I will submit instead two patches: One patch that 
extends the API and another modified version of the current igc patch which will 
use the new more granular option. For the former, I will try and persuade (...beg... I will beg...) 
JL to supervise the API work so it does not go off the rails :-)

In parallel, I have reached out to Kevin and asked if the 1-step logic will ever be able to
update the UDP checksum on-the-fly in which case I shall certainly include this functionality
as well.

Let me know what you think.

All the best,

Lasse

> On 9 Sep 2022, at 15:21, Richard Cochran <richardcochran@gmail.com> wrote:
> 
> On Fri, Sep 09, 2022 at 09:51:23AM +0100, Lasse Johnsen wrote:
> 
>> So where the driver received an ioctl with tx config option HWTSTAMP_TX_ONESTEP_SYNC it will process 
>> skbs not matching the above criteria (including  PTP_CLASS_IPV4) as it would have had the tx config option 
>> been HWTSTAMP_TX_ON. This patch does not change the behaviour of the latter in any way.
>> 
>> Therefore a user space application which has used the HWTSTAMP_TX_ONESTEP_SYNC tx config option
>> and is sending UDP messages will as usual receive those messages back in the error queue with 
>> hardware timestamps in the normal fashion. (provided of course in the case of this specific driver that
>> another tx timestamping operation is not in progress.)
> 
> Okay, then I must NAK this patch.  If driver offers one-step and user
> enables it, then it should work.
> 
> The option, HWTSTAMP_TX_ONESTEP_SYNC, means to apply one-step to any
> and all Sync frames, not just layer 2.  The API does not offer a way
> to advertise or configure one-step selectively based on network layer.
> 
> Thanks,
> Richard


^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-12 15:44 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-08 22:18 [PATCH net-next 1/1] igc: ptp: Add 1-step functionality to igc driver Lasse Johnsen
2022-09-08 22:28 ` Richard Cochran
2022-09-08 22:48   ` Lasse Johnsen
2022-09-09  2:51     ` Richard Cochran
2022-09-09  8:51       ` Lasse Johnsen
2022-09-09 14:21         ` Richard Cochran
2022-09-09 15:53 ` kernel test robot
  -- strict thread matches above, loose matches on Subject: below --
2022-09-10 15:07 Lasse Johnsen
2022-09-12 15:43 ` Richard Cochran

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).