[PATCH net 0/6][pull request] Intel Wired LAN Driver Updates 2023-07-05 (igc)

[PATCH net 0/6][pull request] Intel Wired LAN Driver Updates 2023-07-05 (igc)

[Tony Nguyen]

This series contains updates to igc driver only.

Husaini adds check to increment Qbv change error counter only on taprio
Qbvs. He also removes delay during Tx ring configuration and
resolves Tx hang that could occur when transmitting on a gate to be
closed.

Prasad Koya reports ethtool link mode as TP (twisted pair).

Tee Min corrects value for max SDU.

Aravindhan ensures that registers for PPS are always programmed to occur
in future.

The following are changes since commit c451410ca7e3d8eeb31d141fc20c200e21754ba4:
  Merge branch 'mptcp-fixes'
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 1GbE

Aravindhan Gunasekaran (1):
  igc: Handle PPS start time programming for past time values

Muhammad Husaini Zulkifli (3):
  igc: Add condition for qbv_config_change_errors counter
  igc: Remove delay during TX ring configuration
  igc: Fix TX Hang issue when QBV Gate is closed

Prasad Koya (1):
  igc: set TP bit in 'supported' and 'advertising' fields of
    ethtool_link_ksettings

Tan Tee Min (1):
  igc: Include the length/type field and VLAN tag in queueMaxSDU

 drivers/net/ethernet/intel/igc/igc.h         |  7 ++
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  2 +
 drivers/net/ethernet/intel/igc/igc_main.c    | 74 ++++++++++++++++----
 drivers/net/ethernet/intel/igc/igc_ptp.c     | 25 ++++++-
 drivers/net/ethernet/intel/igc/igc_tsn.c     | 42 +++++++----
 5 files changed, 118 insertions(+), 32 deletions(-)

-- 
2.38.1

[PATCH net 1/6] igc: Add condition for qbv_config_change_errors counter

[Tony Nguyen]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

Add condition to increase the qbv counter during taprio qbv
configuration only.

There might be a case when TC already been setup then user configure
the ETF/CBS qdisc and this counter will increase if no condition above.

Fixes: ae4fe4698300 ("igc: Add qbv_config_change_errors counter")
Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      | 1 +
 drivers/net/ethernet/intel/igc/igc_main.c | 2 ++
 drivers/net/ethernet/intel/igc/igc_tsn.c  | 1 +
 3 files changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 00a5ee487812..aa5ceab0d371 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -184,6 +184,7 @@ struct igc_adapter {
 	u32 max_frame_size;
 	u32 min_frame_size;
 
+	int tc_setup_type;
 	ktime_t base_time;
 	ktime_t cycle_time;
 	bool qbv_enable;
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 019ce91c45aa..b90f94511005 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6327,6 +6327,8 @@ static int igc_setup_tc(struct net_device *dev, enum tc_setup_type type,
 {
 	struct igc_adapter *adapter = netdev_priv(dev);
 
+	adapter->tc_setup_type = type;
+
 	switch (type) {
 	case TC_QUERY_CAPS:
 		return igc_tc_query_caps(adapter, type_data);
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index 94a2b0dfb54d..6b299b83e7ef 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -249,6 +249,7 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 		 * Gate Control List (GCL) is running.
 		 */
 		if ((rd32(IGC_BASET_H) || rd32(IGC_BASET_L)) &&
+		    (adapter->tc_setup_type == TC_SETUP_QDISC_TAPRIO) &&
 		    tsn_mode_reconfig)
 			adapter->qbv_config_change_errors++;
 	} else {
-- 
2.38.1

[PATCH net 2/6] igc: Remove delay during TX ring configuration

[Tony Nguyen]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

Remove unnecessary delay during the TX ring configuration.
This will cause delay, especially during link down and
link up activity.

Furthermore, old SKUs like as I225 will call the reset_adapter
to reset the controller during TSN mode Gate Control List (GCL)
setting. This will add more time to the configuration of the
real-time use case.

It doesn't mentioned about this delay in the Software User Manual.
It might have been ported from legacy code I210 in the past.

Fixes: 13b5b7fd6a4a ("igc: Add support for Tx/Rx rings")
Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index b90f94511005..7e22204822ea 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -711,7 +711,6 @@ static void igc_configure_tx_ring(struct igc_adapter *adapter,
 	/* disable the queue */
 	wr32(IGC_TXDCTL(reg_idx), 0);
 	wrfl();
-	mdelay(10);
 
 	wr32(IGC_TDLEN(reg_idx),
 	     ring->count * sizeof(union igc_adv_tx_desc));
-- 
2.38.1

[PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed

[Tony Nguyen]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

If a user schedules a Gate Control List (GCL) to close one of
the QBV gates while also transmitting a packet to that closed gate,
TX Hang will be happen. HW would not drop any packet when the gate
is closed and keep queuing up in HW TX FIFO until the gate is re-opened.
This patch implements the solution to drop the packet for the closed
gate.

This patch will also reset the adapter to perform SW initialization
for each 1st Gate Control List (GCL) to avoid hang.
This is due to the HW design, where changing to TSN transmit mode
requires SW initialization. Intel Discrete I225/6 transmit mode
cannot be changed when in dynamic mode according to Software User
Manual Section 7.5.2.1. Subsequent Gate Control List (GCL) operations
will proceed without a reset, as they already are in TSN Mode.

Step to reproduce:

DUT:
1) Configure GCL List with certain gate close.

BASE=$(date +%s%N)
tc qdisc replace dev $IFACE parent root handle 100 taprio \
    num_tc 4 \
    map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
    queues 1@0 1@1 1@2 1@3 \
    base-time $BASE \
    sched-entry S 0x8 500000 \
    sched-entry S 0x4 500000 \
    flags 0x2

2) Transmit the packet to closed gate. You may use udp_tai
application to transmit UDP packet to any of the closed gate.

./udp_tai -i <interface> -P 100000 -p 90 -c 1 -t <0/1> -u 30004

Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading")
Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com>
Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com>
Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com>
Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      |  6 +++
 drivers/net/ethernet/intel/igc/igc_main.c | 58 +++++++++++++++++++++--
 drivers/net/ethernet/intel/igc/igc_tsn.c  | 41 ++++++++++------
 3 files changed, 87 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index aa5ceab0d371..639a50c02537 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -14,6 +14,7 @@
 #include <linux/timecounter.h>
 #include <linux/net_tstamp.h>
 #include <linux/bitfield.h>
+#include <linux/hrtimer.h>
 
 #include "igc_hw.h"
 
@@ -101,6 +102,8 @@ struct igc_ring {
 	u32 start_time;
 	u32 end_time;
 	u32 max_sdu;
+	bool oper_gate_closed;		/* Operating gate. True if the TX Queue is closed */
+	bool admin_gate_closed;		/* Future gate. True if the TX Queue will be closed */
 
 	/* CBS parameters */
 	bool cbs_enable;                /* indicates if CBS is enabled */
@@ -160,6 +163,7 @@ struct igc_adapter {
 	struct timer_list watchdog_timer;
 	struct timer_list dma_err_timer;
 	struct timer_list phy_info_timer;
+	struct hrtimer hrtimer;
 
 	u32 wol;
 	u32 en_mng_pt;
@@ -189,6 +193,8 @@ struct igc_adapter {
 	ktime_t cycle_time;
 	bool qbv_enable;
 	u32 qbv_config_change_errors;
+	bool qbv_transition;
+	unsigned int qbv_count;
 
 	/* OS defined structs */
 	struct pci_dev *pdev;
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 7e22204822ea..e5bfc4000658 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1572,6 +1572,9 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 	first->bytecount = skb->len;
 	first->gso_segs = 1;
 
+	if (adapter->qbv_transition || tx_ring->oper_gate_closed)
+		goto out_drop;
+
 	if (tx_ring->max_sdu > 0) {
 		u32 max_sdu = 0;
 
@@ -3011,8 +3014,8 @@ static bool igc_clean_tx_irq(struct igc_q_vector *q_vector, int napi_budget)
 		    time_after(jiffies, tx_buffer->time_stamp +
 		    (adapter->tx_timeout_factor * HZ)) &&
 		    !(rd32(IGC_STATUS) & IGC_STATUS_TXOFF) &&
-		    (rd32(IGC_TDH(tx_ring->reg_idx)) !=
-		     readl(tx_ring->tail))) {
+		    (rd32(IGC_TDH(tx_ring->reg_idx)) != readl(tx_ring->tail)) &&
+		    !tx_ring->oper_gate_closed) {
 			/* detected Tx unit hang */
 			netdev_err(tx_ring->netdev,
 				   "Detected Tx Unit Hang\n"
@@ -6102,6 +6105,8 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter)
 	adapter->base_time = 0;
 	adapter->cycle_time = NSEC_PER_SEC;
 	adapter->qbv_config_change_errors = 0;
+	adapter->qbv_transition = false;
+	adapter->qbv_count = 0;
 
 	for (i = 0; i < adapter->num_tx_queues; i++) {
 		struct igc_ring *ring = adapter->tx_ring[i];
@@ -6109,6 +6114,8 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter)
 		ring->start_time = 0;
 		ring->end_time = NSEC_PER_SEC;
 		ring->max_sdu = 0;
+		ring->oper_gate_closed = false;
+		ring->admin_gate_closed = false;
 	}
 
 	return 0;
@@ -6120,6 +6127,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 	bool queue_configured[IGC_MAX_TX_QUEUES] = { };
 	struct igc_hw *hw = &adapter->hw;
 	u32 start_time = 0, end_time = 0;
+	struct timespec64 now;
 	size_t n;
 	int i;
 
@@ -6149,6 +6157,8 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 	adapter->cycle_time = qopt->cycle_time;
 	adapter->base_time = qopt->base_time;
 
+	igc_ptp_read(adapter, &now);
+
 	for (n = 0; n < qopt->num_entries; n++) {
 		struct tc_taprio_sched_entry *e = &qopt->entries[n];
 
@@ -6183,7 +6193,10 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 				ring->start_time = start_time;
 			ring->end_time = end_time;
 
-			queue_configured[i] = true;
+			if (ring->start_time >= adapter->cycle_time)
+				queue_configured[i] = false;
+			else
+				queue_configured[i] = true;
 		}
 
 		start_time += e->interval;
@@ -6193,8 +6206,20 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 	 * If not, set the start and end time to be end time.
 	 */
 	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct igc_ring *ring = adapter->tx_ring[i];
+
+		if (!is_base_time_past(qopt->base_time, &now)) {
+			ring->admin_gate_closed = false;
+		} else {
+			ring->oper_gate_closed = false;
+			ring->admin_gate_closed = false;
+		}
+
 		if (!queue_configured[i]) {
-			struct igc_ring *ring = adapter->tx_ring[i];
+			if (!is_base_time_past(qopt->base_time, &now))
+				ring->admin_gate_closed = true;
+			else
+				ring->oper_gate_closed = true;
 
 			ring->start_time = end_time;
 			ring->end_time = end_time;
@@ -6575,6 +6600,27 @@ static const struct xdp_metadata_ops igc_xdp_metadata_ops = {
 	.xmo_rx_timestamp		= igc_xdp_rx_timestamp,
 };
 
+static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer)
+{
+	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
+						   hrtimer);
+	unsigned int i;
+
+	adapter->qbv_transition = true;
+	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct igc_ring *tx_ring = adapter->tx_ring[i];
+
+		if (tx_ring->admin_gate_closed) {
+			tx_ring->admin_gate_closed = false;
+			tx_ring->oper_gate_closed = true;
+		} else {
+			tx_ring->oper_gate_closed = false;
+		}
+	}
+	adapter->qbv_transition = false;
+	return HRTIMER_NORESTART;
+}
+
 /**
  * igc_probe - Device Initialization Routine
  * @pdev: PCI device information struct
@@ -6753,6 +6799,9 @@ static int igc_probe(struct pci_dev *pdev,
 	INIT_WORK(&adapter->reset_task, igc_reset_task);
 	INIT_WORK(&adapter->watchdog_task, igc_watchdog_task);
 
+	hrtimer_init(&adapter->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	adapter->hrtimer.function = &igc_qbv_scheduling_timer;
+
 	/* Initialize link properties that are user-changeable */
 	adapter->fc_autoneg = true;
 	hw->mac.autoneg = true;
@@ -6856,6 +6905,7 @@ static void igc_remove(struct pci_dev *pdev)
 
 	cancel_work_sync(&adapter->reset_task);
 	cancel_work_sync(&adapter->watchdog_task);
+	hrtimer_cancel(&adapter->hrtimer);
 
 	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
 	 * would have already happened in close and is redundant.
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index 6b299b83e7ef..3cdb0c988728 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -114,7 +114,6 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter)
 static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
-	bool tsn_mode_reconfig = false;
 	u32 tqavctrl, baset_l, baset_h;
 	u32 sec, nsec, cycle;
 	ktime_t base_time, systim;
@@ -228,11 +227,10 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 
 	tqavctrl = rd32(IGC_TQAVCTRL) & ~IGC_TQAVCTRL_FUTSCDDIS;
 
-	if (tqavctrl & IGC_TQAVCTRL_TRANSMIT_MODE_TSN)
-		tsn_mode_reconfig = true;
-
 	tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV;
 
+	adapter->qbv_count++;
+
 	cycle = adapter->cycle_time;
 	base_time = adapter->base_time;
 
@@ -250,17 +248,28 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 		 */
 		if ((rd32(IGC_BASET_H) || rd32(IGC_BASET_L)) &&
 		    (adapter->tc_setup_type == TC_SETUP_QDISC_TAPRIO) &&
-		    tsn_mode_reconfig)
+		    (adapter->qbv_count > 1))
 			adapter->qbv_config_change_errors++;
 	} else {
-		/* According to datasheet section 7.5.2.9.3.3, FutScdDis bit
-		 * has to be configured before the cycle time and base time.
-		 * Tx won't hang if there is a GCL is already running,
-		 * so in this case we don't need to set FutScdDis.
-		 */
-		if (igc_is_device_id_i226(hw) &&
-		    !(rd32(IGC_BASET_H) || rd32(IGC_BASET_L)))
-			tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS;
+		if (igc_is_device_id_i226(hw)) {
+			ktime_t adjust_time, expires_time;
+
+		       /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit
+			* has to be configured before the cycle time and base time.
+			* Tx won't hang if a GCL is already running,
+			* so in this case we don't need to set FutScdDis.
+			*/
+			if (!(rd32(IGC_BASET_H) || rd32(IGC_BASET_L)))
+				tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS;
+
+			nsec = rd32(IGC_SYSTIML);
+			sec = rd32(IGC_SYSTIMH);
+			systim = ktime_set(sec, nsec);
+
+			adjust_time = adapter->base_time;
+			expires_time = ktime_sub_ns(adjust_time, systim);
+			hrtimer_start(&adapter->hrtimer, expires_time, HRTIMER_MODE_REL);
+		}
 	}
 
 	wr32(IGC_TQAVCTRL, tqavctrl);
@@ -306,7 +315,11 @@ int igc_tsn_offload_apply(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
 
-	if (netif_running(adapter->netdev) && igc_is_device_id_i225(hw)) {
+	/* Per I225/6 HW Design Section 7.5.2.1, transmit mode
+	 * cannot be changed dynamically. Require reset the adapter.
+	 */
+	if (netif_running(adapter->netdev) &&
+	    (igc_is_device_id_i225(hw) || !adapter->qbv_count)) {
 		schedule_work(&adapter->reset_task);
 		return 0;
 	}
-- 
2.38.1

[PATCH net 4/6] igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings

[Tony Nguyen]

From: Prasad Koya <prasad@arista.com>

set TP bit in the 'supported' and 'advertising' fields. i225/226 parts
only support twisted pair copper.

Fixes: 8c5ad0dae93c ("igc: Add ethtool support")
Signed-off-by: Prasad Koya <prasad@arista.com>
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 0e2cb00622d1..93bce729be76 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1708,6 +1708,8 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
 	/* twisted pair */
 	cmd->base.port = PORT_TP;
 	cmd->base.phy_address = hw->phy.addr;
+	ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
+	ethtool_link_ksettings_add_link_mode(cmd, advertising, TP);
 
 	/* advertising link modes */
 	if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF)
-- 
2.38.1

[PATCH net 5/6] igc: Include the length/type field and VLAN tag in queueMaxSDU

[Tony Nguyen]

From: Tan Tee Min <tee.min.tan@linux.intel.com>

IEEE 802.1Q does not have clear definitions of what constitutes an
SDU (Service Data Unit), but IEEE Std 802.3 clause 3.1.2 does define
the MAC service primitives and clause 3.2.7 does define the MAC Client
Data for Q-tagged frames.

It shows that the mac_service_data_unit (MSDU) does NOT contain the
preamble, destination and source address, or FCS. The MSDU does contain
the length/type field, MAC client data, VLAN tag and any padding
data (prior to the FCS).

Thus, the maximum 802.3 frame size that is allowed to be transmitted
should be QueueMaxSDU (MSDU) + 16 (6 byte SA + 6 byte DA + 4 byte FCS).

Fixes: 92a0dcb8427d ("igc: offload queue max SDU from tc-taprio")
Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com>
Reviewed-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index e5bfc4000658..281a0e35b9d1 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1575,16 +1575,9 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 	if (adapter->qbv_transition || tx_ring->oper_gate_closed)
 		goto out_drop;
 
-	if (tx_ring->max_sdu > 0) {
-		u32 max_sdu = 0;
-
-		max_sdu = tx_ring->max_sdu +
-			  (skb_vlan_tagged(first->skb) ? VLAN_HLEN : 0);
-
-		if (first->bytecount > max_sdu) {
-			adapter->stats.txdrop++;
-			goto out_drop;
-		}
+	if (tx_ring->max_sdu > 0 && first->bytecount > tx_ring->max_sdu) {
+		adapter->stats.txdrop++;
+		goto out_drop;
 	}
 
 	if (unlikely(test_bit(IGC_RING_FLAG_TX_HWTSTAMP, &tx_ring->flags) &&
@@ -6231,7 +6224,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 		struct net_device *dev = adapter->netdev;
 
 		if (qopt->max_sdu[i])
-			ring->max_sdu = qopt->max_sdu[i] + dev->hard_header_len;
+			ring->max_sdu = qopt->max_sdu[i] + dev->hard_header_len - ETH_TLEN;
 		else
 			ring->max_sdu = 0;
 	}
-- 
2.38.1

[PATCH net 6/6] igc: Handle PPS start time programming for past time values

[Tony Nguyen]

From: Aravindhan Gunasekaran <aravindhan.gunasekaran@intel.com>

I225/6 hardware can be programmed to start PPS output once
the time in Target Time registers is reached. The time
programmed in these registers should always be into future.
Only then PPS output is triggered when SYSTIM register
reaches the programmed value. There are two modes in i225/6
hardware to program PPS, pulse and clock mode.

There were issues reported where PPS is not generated when
start time is in past.

Example 1, "echo 0 0 0 2 0 > /sys/class/ptp/ptp0/period"

In the current implementation, a value of '0' is programmed
into Target time registers and PPS output is in pulse mode.
Eventually an interrupt which is triggered upon SYSTIM
register reaching Target time is not fired. Thus no PPS
output is generated.

Example 2, "echo 0 0 0 1 0 > /sys/class/ptp/ptp0/period"

Above case, a value of '0' is programmed into Target time
registers and PPS output is in clock mode. Here, HW tries to
catch-up the current time by incrementing Target Time
register. This catch-up time seem to vary according to
programmed PPS period time as per the HW design. In my
experiments, the delay ranged between few tens of seconds to
few minutes. The PPS output is only generated after the
Target time register reaches current time.

In my experiments, I also observed PPS stopped working with
below test and could not recover until module is removed and
loaded again.

1) echo 0 <future time> 0 1 0 > /sys/class/ptp/ptp1/period
2) echo 0 0 0 1 0 > /sys/class/ptp/ptp1/period
3) echo 0 0 0 1 0 > /sys/class/ptp/ptp1/period

After this PPS did not work even if i re-program with proper
values. I could only get this back working by reloading the
driver.

This patch takes care of calculating and programming
appropriate future time value into Target Time registers.

Fixes: 5e91c72e560c ("igc: Fix PPS delta between two synchronized end-points")
Signed-off-by: Aravindhan Gunasekaran <aravindhan.gunasekaran@intel.com>
Reviewed-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ptp.c | 25 +++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 32ef112f8291..f0b979a70655 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -356,16 +356,35 @@ static int igc_ptp_feature_enable_i225(struct ptp_clock_info *ptp,
 			tsim &= ~IGC_TSICR_TT0;
 		}
 		if (on) {
+			struct timespec64 safe_start;
 			int i = rq->perout.index;
 
 			igc_pin_perout(igc, i, pin, use_freq);
-			igc->perout[i].start.tv_sec = rq->perout.start.sec;
+			igc_ptp_read(igc, &safe_start);
+
+			/* PPS output start time is triggered by Target time(TT)
+			 * register. Programming any past time value into TT
+			 * register will cause PPS to never start. Need to make
+			 * sure we program the TT register a time ahead in
+			 * future. There isn't a stringent need to fire PPS out
+			 * right away. Adding +2 seconds should take care of
+			 * corner cases. Let's say if the SYSTIML is close to
+			 * wrap up and the timer keeps ticking as we program the
+			 * register, adding +2seconds is safe bet.
+			 */
+			safe_start.tv_sec += 2;
+
+			if (rq->perout.start.sec < safe_start.tv_sec)
+				igc->perout[i].start.tv_sec = safe_start.tv_sec;
+			else
+				igc->perout[i].start.tv_sec = rq->perout.start.sec;
 			igc->perout[i].start.tv_nsec = rq->perout.start.nsec;
 			igc->perout[i].period.tv_sec = ts.tv_sec;
 			igc->perout[i].period.tv_nsec = ts.tv_nsec;
-			wr32(trgttimh, rq->perout.start.sec);
+			wr32(trgttimh, (u32)igc->perout[i].start.tv_sec);
 			/* For now, always select timer 0 as source. */
-			wr32(trgttiml, rq->perout.start.nsec | IGC_TT_IO_TIMER_SEL_SYSTIM0);
+			wr32(trgttiml, (u32)(igc->perout[i].start.tv_nsec |
+					     IGC_TT_IO_TIMER_SEL_SYSTIM0));
 			if (use_freq)
 				wr32(freqout, ns);
 			tsauxc |= tsauxc_mask;
-- 
2.38.1

Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed

[Leon Romanovsky]

On Wed, Jul 05, 2023 at 01:19:02PM -0700, Tony Nguyen wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> 
> If a user schedules a Gate Control List (GCL) to close one of
> the QBV gates while also transmitting a packet to that closed gate,
> TX Hang will be happen. HW would not drop any packet when the gate
> is closed and keep queuing up in HW TX FIFO until the gate is re-opened.
> This patch implements the solution to drop the packet for the closed
> gate.
> 
> This patch will also reset the adapter to perform SW initialization
> for each 1st Gate Control List (GCL) to avoid hang.
> This is due to the HW design, where changing to TSN transmit mode
> requires SW initialization. Intel Discrete I225/6 transmit mode
> cannot be changed when in dynamic mode according to Software User
> Manual Section 7.5.2.1. Subsequent Gate Control List (GCL) operations
> will proceed without a reset, as they already are in TSN Mode.
> 
> Step to reproduce:
> 
> DUT:
> 1) Configure GCL List with certain gate close.
> 
> BASE=$(date +%s%N)
> tc qdisc replace dev $IFACE parent root handle 100 taprio \
>     num_tc 4 \
>     map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
>     queues 1@0 1@1 1@2 1@3 \
>     base-time $BASE \
>     sched-entry S 0x8 500000 \
>     sched-entry S 0x4 500000 \
>     flags 0x2
> 
> 2) Transmit the packet to closed gate. You may use udp_tai
> application to transmit UDP packet to any of the closed gate.
> 
> ./udp_tai -i <interface> -P 100000 -p 90 -c 1 -t <0/1> -u 30004
> 
> Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading")
> Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com>
> Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com>
> Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com>
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h      |  6 +++
>  drivers/net/ethernet/intel/igc/igc_main.c | 58 +++++++++++++++++++++--
>  drivers/net/ethernet/intel/igc/igc_tsn.c  | 41 ++++++++++------
>  3 files changed, 87 insertions(+), 18 deletions(-)

<...>

> @@ -6149,6 +6157,8 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>  	adapter->cycle_time = qopt->cycle_time;
>  	adapter->base_time = qopt->base_time;
>  
> +	igc_ptp_read(adapter, &now);
> +
>  	for (n = 0; n < qopt->num_entries; n++) {
>  		struct tc_taprio_sched_entry *e = &qopt->entries[n];
>  
> @@ -6183,7 +6193,10 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>  				ring->start_time = start_time;
>  			ring->end_time = end_time;
>  
> -			queue_configured[i] = true;
> +			if (ring->start_time >= adapter->cycle_time)
> +				queue_configured[i] = false;
> +			else
> +				queue_configured[i] = true;
>  		}
>  
>  		start_time += e->interval;
> @@ -6193,8 +6206,20 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>  	 * If not, set the start and end time to be end time.
>  	 */
>  	for (i = 0; i < adapter->num_tx_queues; i++) {
> +		struct igc_ring *ring = adapter->tx_ring[i];
> +
> +		if (!is_base_time_past(qopt->base_time, &now)) {
> +			ring->admin_gate_closed = false;
> +		} else {
> +			ring->oper_gate_closed = false;
> +			ring->admin_gate_closed = false;
> +		}
> +
>  		if (!queue_configured[i]) {
> -			struct igc_ring *ring = adapter->tx_ring[i];
> +			if (!is_base_time_past(qopt->base_time, &now))
> +				ring->admin_gate_closed = true;
> +			else
> +				ring->oper_gate_closed = true;
>  
>  			ring->start_time = end_time;
>  			ring->end_time = end_time;
> @@ -6575,6 +6600,27 @@ static const struct xdp_metadata_ops igc_xdp_metadata_ops = {
>  	.xmo_rx_timestamp		= igc_xdp_rx_timestamp,
>  };
>  
> +static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer)
> +{
> +	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
> +						   hrtimer);
> +	unsigned int i;
> +
> +	adapter->qbv_transition = true;
> +	for (i = 0; i < adapter->num_tx_queues; i++) {
> +		struct igc_ring *tx_ring = adapter->tx_ring[i];
> +
> +		if (tx_ring->admin_gate_closed) {

Doesn't asynchronic access to shared variable through hrtimer require some sort of locking?

Thanks

> +			tx_ring->admin_gate_closed = false;
> +			tx_ring->oper_gate_closed = true;
> +		} else {
> +			tx_ring->oper_gate_closed = false;
> +		}
> +	}
> +	adapter->qbv_transition = false;
> +	return HRTIMER_NORESTART;
> +}
> +

RE: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed

[Zulkifli, Muhammad Husaini]

Dear Leon,

Thanks for reviewing 😊
Replied inline.

> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Thursday, 6 July, 2023 3:56 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; netdev@vger.kernel.org; Zulkifli, Muhammad
> Husaini <muhammad.husaini.zulkifli@intel.com>; Neftin, Sasha
> <sasha.neftin@intel.com>; richardcochran@gmail.com; Tan Tee Min
> <tee.min.tan@linux.intel.com>; Choong, Chwee Lin
> <chwee.lin.choong@intel.com>; Naama Meir
> <naamax.meir@linux.intel.com>
> Subject: Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed
> 
> On Wed, Jul 05, 2023 at 01:19:02PM -0700, Tony Nguyen wrote:
> > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> >
> > If a user schedules a Gate Control List (GCL) to close one of the QBV
> > gates while also transmitting a packet to that closed gate, TX Hang
> > will be happen. HW would not drop any packet when the gate is closed
> > and keep queuing up in HW TX FIFO until the gate is re-opened.
> > This patch implements the solution to drop the packet for the closed
> > gate.
> >
> > This patch will also reset the adapter to perform SW initialization
> > for each 1st Gate Control List (GCL) to avoid hang.
> > This is due to the HW design, where changing to TSN transmit mode
> > requires SW initialization. Intel Discrete I225/6 transmit mode cannot
> > be changed when in dynamic mode according to Software User Manual
> > Section 7.5.2.1. Subsequent Gate Control List (GCL) operations will
> > proceed without a reset, as they already are in TSN Mode.
> >
> > Step to reproduce:
> >
> > DUT:
> > 1) Configure GCL List with certain gate close.
> >
> > BASE=$(date +%s%N)
> > tc qdisc replace dev $IFACE parent root handle 100 taprio \
> >     num_tc 4 \
> >     map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
> >     queues 1@0 1@1 1@2 1@3 \
> >     base-time $BASE \
> >     sched-entry S 0x8 500000 \
> >     sched-entry S 0x4 500000 \
> >     flags 0x2
> >
> > 2) Transmit the packet to closed gate. You may use udp_tai application
> > to transmit UDP packet to any of the closed gate.
> >
> > ./udp_tai -i <interface> -P 100000 -p 90 -c 1 -t <0/1> -u 30004
> >
> > Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading")
> > Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com>
> > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com>
> > Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com>
> > Signed-off-by: Muhammad Husaini Zulkifli
> > <muhammad.husaini.zulkifli@intel.com>
> > Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc.h      |  6 +++
> >  drivers/net/ethernet/intel/igc/igc_main.c | 58
> > +++++++++++++++++++++--  drivers/net/ethernet/intel/igc/igc_tsn.c  |
> > 41 ++++++++++------
> >  3 files changed, 87 insertions(+), 18 deletions(-)
> 
> <...>
> 
> > @@ -6149,6 +6157,8 @@ static int igc_save_qbv_schedule(struct
> igc_adapter *adapter,
> >  	adapter->cycle_time = qopt->cycle_time;
> >  	adapter->base_time = qopt->base_time;
> >
> > +	igc_ptp_read(adapter, &now);
> > +
> >  	for (n = 0; n < qopt->num_entries; n++) {
> >  		struct tc_taprio_sched_entry *e = &qopt->entries[n];
> >
> > @@ -6183,7 +6193,10 @@ static int igc_save_qbv_schedule(struct
> igc_adapter *adapter,
> >  				ring->start_time = start_time;
> >  			ring->end_time = end_time;
> >
> > -			queue_configured[i] = true;
> > +			if (ring->start_time >= adapter->cycle_time)
> > +				queue_configured[i] = false;
> > +			else
> > +				queue_configured[i] = true;
> >  		}
> >
> >  		start_time += e->interval;
> > @@ -6193,8 +6206,20 @@ static int igc_save_qbv_schedule(struct
> igc_adapter *adapter,
> >  	 * If not, set the start and end time to be end time.
> >  	 */
> >  	for (i = 0; i < adapter->num_tx_queues; i++) {
> > +		struct igc_ring *ring = adapter->tx_ring[i];
> > +
> > +		if (!is_base_time_past(qopt->base_time, &now)) {
> > +			ring->admin_gate_closed = false;
> > +		} else {
> > +			ring->oper_gate_closed = false;
> > +			ring->admin_gate_closed = false;
> > +		}
> > +
> >  		if (!queue_configured[i]) {
> > -			struct igc_ring *ring = adapter->tx_ring[i];
> > +			if (!is_base_time_past(qopt->base_time, &now))
> > +				ring->admin_gate_closed = true;
> > +			else
> > +				ring->oper_gate_closed = true;
> >
> >  			ring->start_time = end_time;
> >  			ring->end_time = end_time;
> > @@ -6575,6 +6600,27 @@ static const struct xdp_metadata_ops
> igc_xdp_metadata_ops = {
> >  	.xmo_rx_timestamp		= igc_xdp_rx_timestamp,
> >  };
> >
> > +static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer
> > +*timer) {
> > +	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
> > +						   hrtimer);
> > +	unsigned int i;
> > +
> > +	adapter->qbv_transition = true;
> > +	for (i = 0; i < adapter->num_tx_queues; i++) {
> > +		struct igc_ring *tx_ring = adapter->tx_ring[i];
> > +
> > +		if (tx_ring->admin_gate_closed) {
> 
> Doesn't asynchronic access to shared variable through hrtimer require some
> sort of locking?

Yeah I agreed with you. However, IMHO, it should be saved without the lock. 
These variables, admin_gate_closed and oper_gate_closed, were set during the transition 
and setup/delete of the TC only. The qbv_transition flag has been used to protect the 
operation when it is in qbv transition.

Thanks,
Husaini

> 
> Thanks
> 
> > +			tx_ring->admin_gate_closed = false;
> > +			tx_ring->oper_gate_closed = true;
> > +		} else {
> > +			tx_ring->oper_gate_closed = false;
> > +		}
> > +	}
> > +	adapter->qbv_transition = false;
> > +	return HRTIMER_NORESTART;
> > +}
> > +

Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed

[Leon Romanovsky]

On Thu, Jul 06, 2023 at 12:43:33PM +0000, Zulkifli, Muhammad Husaini wrote:
> Dear Leon,
> 
> Thanks for reviewing 😊
> Replied inline.
> 
> > -----Original Message-----
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Thursday, 6 July, 2023 3:56 PM
> > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> > edumazet@google.com; netdev@vger.kernel.org; Zulkifli, Muhammad
> > Husaini <muhammad.husaini.zulkifli@intel.com>; Neftin, Sasha
> > <sasha.neftin@intel.com>; richardcochran@gmail.com; Tan Tee Min
> > <tee.min.tan@linux.intel.com>; Choong, Chwee Lin
> > <chwee.lin.choong@intel.com>; Naama Meir
> > <naamax.meir@linux.intel.com>
> > Subject: Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed
> > 
> > On Wed, Jul 05, 2023 at 01:19:02PM -0700, Tony Nguyen wrote:
> > > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> > >
> > > If a user schedules a Gate Control List (GCL) to close one of the QBV
> > > gates while also transmitting a packet to that closed gate, TX Hang
> > > will be happen. HW would not drop any packet when the gate is closed
> > > and keep queuing up in HW TX FIFO until the gate is re-opened.
> > > This patch implements the solution to drop the packet for the closed
> > > gate.
> > >
> > > This patch will also reset the adapter to perform SW initialization
> > > for each 1st Gate Control List (GCL) to avoid hang.
> > > This is due to the HW design, where changing to TSN transmit mode
> > > requires SW initialization. Intel Discrete I225/6 transmit mode cannot
> > > be changed when in dynamic mode according to Software User Manual
> > > Section 7.5.2.1. Subsequent Gate Control List (GCL) operations will
> > > proceed without a reset, as they already are in TSN Mode.
> > >
> > > Step to reproduce:
> > >
> > > DUT:
> > > 1) Configure GCL List with certain gate close.
> > >
> > > BASE=$(date +%s%N)
> > > tc qdisc replace dev $IFACE parent root handle 100 taprio \
> > >     num_tc 4 \
> > >     map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
> > >     queues 1@0 1@1 1@2 1@3 \
> > >     base-time $BASE \
> > >     sched-entry S 0x8 500000 \
> > >     sched-entry S 0x4 500000 \
> > >     flags 0x2
> > >
> > > 2) Transmit the packet to closed gate. You may use udp_tai application
> > > to transmit UDP packet to any of the closed gate.
> > >
> > > ./udp_tai -i <interface> -P 100000 -p 90 -c 1 -t <0/1> -u 30004
> > >
> > > Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading")
> > > Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com>
> > > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com>
> > > Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com>
> > > Signed-off-by: Muhammad Husaini Zulkifli
> > > <muhammad.husaini.zulkifli@intel.com>
> > > Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/igc/igc.h      |  6 +++
> > >  drivers/net/ethernet/intel/igc/igc_main.c | 58
> > > +++++++++++++++++++++--  drivers/net/ethernet/intel/igc/igc_tsn.c  |
> > > 41 ++++++++++------
> > >  3 files changed, 87 insertions(+), 18 deletions(-)

<...>

> > > +static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer
> > > +*timer) {
> > > +	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
> > > +						   hrtimer);
> > > +	unsigned int i;
> > > +
> > > +	adapter->qbv_transition = true;
> > > +	for (i = 0; i < adapter->num_tx_queues; i++) {
> > > +		struct igc_ring *tx_ring = adapter->tx_ring[i];
> > > +
> > > +		if (tx_ring->admin_gate_closed) {
> > 
> > Doesn't asynchronic access to shared variable through hrtimer require some
> > sort of locking?
> 
> Yeah I agreed with you. However, IMHO, it should be saved without the lock. 
> These variables, admin_gate_closed and oper_gate_closed, were set during the transition 
> and setup/delete of the TC only. The qbv_transition flag has been used to protect the 
> operation when it is in qbv transition.

I have no idea what last sentence means, but igc_qbv_scheduling_timer()
and tx_ring are global function/variables and TC setup/delete can run in
parallel to them.

Thanks

RE: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed

[Zulkifli, Muhammad Husaini]

> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Friday, 7 July, 2023 12:41 AM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> netdev@vger.kernel.org; Neftin, Sasha <sasha.neftin@intel.com>;
> richardcochran@gmail.com; Tan Tee Min <tee.min.tan@linux.intel.com>;
> Choong, Chwee Lin <chwee.lin.choong@intel.com>; Naama Meir
> <naamax.meir@linux.intel.com>
> Subject: Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is closed
> 
> On Thu, Jul 06, 2023 at 12:43:33PM +0000, Zulkifli, Muhammad Husaini wrote:
> > Dear Leon,
> >
> > Thanks for reviewing 😊
> > Replied inline.
> >
> > > -----Original Message-----
> > > From: Leon Romanovsky <leon@kernel.org>
> > > Sent: Thursday, 6 July, 2023 3:56 PM
> > > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> > > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> > > edumazet@google.com; netdev@vger.kernel.org; Zulkifli, Muhammad
> > > Husaini <muhammad.husaini.zulkifli@intel.com>; Neftin, Sasha
> > > <sasha.neftin@intel.com>; richardcochran@gmail.com; Tan Tee Min
> > > <tee.min.tan@linux.intel.com>; Choong, Chwee Lin
> > > <chwee.lin.choong@intel.com>; Naama Meir
> > > <naamax.meir@linux.intel.com>
> > > Subject: Re: [PATCH net 3/6] igc: Fix TX Hang issue when QBV Gate is
> > > closed
> > >
> > > On Wed, Jul 05, 2023 at 01:19:02PM -0700, Tony Nguyen wrote:
> > > > From: Muhammad Husaini Zulkifli
> > > > <muhammad.husaini.zulkifli@intel.com>
> > > >
> > > > If a user schedules a Gate Control List (GCL) to close one of the
> > > > QBV gates while also transmitting a packet to that closed gate, TX
> > > > Hang will be happen. HW would not drop any packet when the gate is
> > > > closed and keep queuing up in HW TX FIFO until the gate is re-opened.
> > > > This patch implements the solution to drop the packet for the
> > > > closed gate.
> > > >
> > > > This patch will also reset the adapter to perform SW
> > > > initialization for each 1st Gate Control List (GCL) to avoid hang.
> > > > This is due to the HW design, where changing to TSN transmit mode
> > > > requires SW initialization. Intel Discrete I225/6 transmit mode
> > > > cannot be changed when in dynamic mode according to Software User
> > > > Manual Section 7.5.2.1. Subsequent Gate Control List (GCL)
> > > > operations will proceed without a reset, as they already are in TSN Mode.
> > > >
> > > > Step to reproduce:
> > > >
> > > > DUT:
> > > > 1) Configure GCL List with certain gate close.
> > > >
> > > > BASE=$(date +%s%N)
> > > > tc qdisc replace dev $IFACE parent root handle 100 taprio \
> > > >     num_tc 4 \
> > > >     map 0 1 2 3 3 3 3 3 3 3 3 3 3 3 3 3 \
> > > >     queues 1@0 1@1 1@2 1@3 \
> > > >     base-time $BASE \
> > > >     sched-entry S 0x8 500000 \
> > > >     sched-entry S 0x4 500000 \
> > > >     flags 0x2
> > > >
> > > > 2) Transmit the packet to closed gate. You may use udp_tai
> > > > application to transmit UDP packet to any of the closed gate.
> > > >
> > > > ./udp_tai -i <interface> -P 100000 -p 90 -c 1 -t <0/1> -u 30004
> > > >
> > > > Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading")
> > > > Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com>
> > > > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com>
> > > > Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com>
> > > > Signed-off-by: Muhammad Husaini Zulkifli
> > > > <muhammad.husaini.zulkifli@intel.com>
> > > > Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> > > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > > ---
> > > >  drivers/net/ethernet/intel/igc/igc.h      |  6 +++
> > > >  drivers/net/ethernet/intel/igc/igc_main.c | 58
> > > > +++++++++++++++++++++--  drivers/net/ethernet/intel/igc/igc_tsn.c
> > > > +++++++++++++++++++++|
> > > > 41 ++++++++++------
> > > >  3 files changed, 87 insertions(+), 18 deletions(-)
> 
> <...>
> 
> > > > +static enum hrtimer_restart igc_qbv_scheduling_timer(struct
> > > > +hrtimer
> > > > +*timer) {
> > > > +	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
> > > > +						   hrtimer);
> > > > +	unsigned int i;
> > > > +
> > > > +	adapter->qbv_transition = true;
> > > > +	for (i = 0; i < adapter->num_tx_queues; i++) {
> > > > +		struct igc_ring *tx_ring = adapter->tx_ring[i];
> > > > +
> > > > +		if (tx_ring->admin_gate_closed) {
> > >
> > > Doesn't asynchronic access to shared variable through hrtimer
> > > require some sort of locking?
> >
> > Yeah I agreed with you. However, IMHO, it should be saved without the lock.
> > These variables, admin_gate_closed and oper_gate_closed, were set
> > during the transition and setup/delete of the TC only. The
> > qbv_transition flag has been used to protect the operation when it is in qbv
> transition.
> 
> I have no idea what last sentence means, but igc_qbv_scheduling_timer() and
> tx_ring are global function/variables and TC setup/delete can run in parallel to
> them.
> 

Thanks for pointing it out. I will add the lock here. 
Will send out new version. Testing it right now.

Thanks,
Husaini

> Thanks