[PATCH net v2 0/6] igc: Fix corner cases for TSN offload

[PATCH net v2 0/6] igc: Fix corner cases for TSN offload

[Florian Kauer]

The igc driver supports several different offloading capabilities
relevant in the TSN context. Recent patches in this area introduced
regressions for certain corner cases that are fixed in this series.

Each of the patches (except the first one) addresses a different
regression that can be separately reproduced. Still, they have
overlapping code changes so they should not be separately applied.

Especially #4 and #6 address the same observation,
but both need to be applied to avoid TX hang occurrences in
the scenario described in the patches.

Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>

---

v2: - Rebased onto net. #1-#2 needed adaptations, others unmodified.
    - Extend #3 commit message that it only regards i225.

---

Florian Kauer (6):
  igc: Rename qbv_enable to taprio_offload_enable
  igc: Do not enable taprio offload for invalid arguments
  igc: Handle already enabled taprio offload for basetime 0
  igc: No strict mode in pure launchtime/CBS offload
  igc: Fix launchtime before start of cycle
  igc: Fix inserting of empty frame for launchtime

 drivers/net/ethernet/intel/igc/igc.h      |  2 +-
 drivers/net/ethernet/intel/igc/igc_main.c | 10 ++++-----
 drivers/net/ethernet/intel/igc/igc_tsn.c  | 26 ++++++++++++++++++++---
 3 files changed, 29 insertions(+), 9 deletions(-)

-- 
2.39.2

[PATCH net v2 1/6] igc: Rename qbv_enable to taprio_offload_enable

[Florian Kauer]

In the current implementation the flags adapter->qbv_enable
and IGC_FLAG_TSN_QBV_ENABLED have a similar name, but do not
have the same meaning. The first one is used only to indicate
taprio offload (i.e. when igc_save_qbv_schedule was called),
while the second one corresponds to the Qbv mode of the hardware.
However, the second one is also used to support the TX launchtime
feature, i.e. ETF qdisc offload. This leads to situations where
adapter->qbv_enable is false, but the flag IGC_FLAG_TSN_QBV_ENABLED
is set. This is prone to confusion.

The rename should reduce this confusion. Since it is a pure
rename, it has no impact on functionality.

Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igc/igc.h      | 2 +-
 drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
 drivers/net/ethernet/intel/igc/igc_tsn.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 34aebf00a512..4c7a1e92ce4c 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -185,7 +185,7 @@ struct igc_adapter {
 
 	ktime_t base_time;
 	ktime_t cycle_time;
-	bool qbv_enable;
+	bool taprio_offload_enable;
 	u32 qbv_config_change_errors;
 
 	/* OS defined structs */
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index fa764190f270..dda057a3b5e3 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6075,7 +6075,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 	size_t n;
 	int i;
 
-	adapter->qbv_enable = qopt->enable;
+	adapter->taprio_offload_enable = qopt->enable;
 
 	if (!qopt->enable)
 		return igc_tsn_clear_schedule(adapter);
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index 94a2b0dfb54d..c6636a7264d5 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -37,7 +37,7 @@ static unsigned int igc_tsn_new_flags(struct igc_adapter *adapter)
 {
 	unsigned int new_flags = adapter->flags & ~IGC_FLAG_TSN_ANY_ENABLED;
 
-	if (adapter->qbv_enable)
+	if (adapter->taprio_offload_enable)
 		new_flags |= IGC_FLAG_TSN_QBV_ENABLED;
 
 	if (is_any_launchtime(adapter))
-- 
2.39.2

[PATCH net v2 2/6] igc: Do not enable taprio offload for invalid arguments

[Florian Kauer]

Only set adapter->taprio_offload_enable after validating the arguments.
Otherwise, it stays set even if the offload was not enabled.
Since the subsequent code does not get executed in case of invalid
arguments, it will not be read at first.
However, by activating and then deactivating another offload
(e.g. ETF/TX launchtime offload), taprio_offload_enable is read
and erroneously keeps the offload feature of the NIC enabled.

This can be reproduced as follows:

    # TAPRIO offload (flags == 0x2) and negative base-time leading to expected -ERANGE
    sudo tc qdisc replace dev enp1s0 parent root handle 100 stab overhead 24 taprio \
	    num_tc 1 \
	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
	    queues 1@0 \
	    base-time -1000 \
	    sched-entry S 01 300000 \
	    flags 0x2

    # IGC_TQAVCTRL is 0x0 as expected (iomem=relaxed for reading register)
    sudo pcimem /sys/bus/pci/devices/0000:01:00.0/resource0 0x3570 w*1

    # Activate ETF offload
    sudo tc qdisc replace dev enp1s0 parent root handle 6666 mqprio \
	    num_tc 3 \
	    map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \
	    queues 1@0 1@1 2@2 \
	    hw 0
    sudo tc qdisc add dev enp1s0 parent 6666:1 etf \
	    clockid CLOCK_TAI \
	    delta 500000 \
	    offload

    # IGC_TQAVCTRL is 0x9 as expected
    sudo pcimem /sys/bus/pci/devices/0000:01:00.0/resource0 0x3570 w*1

    # Deactivate ETF offload again
    sudo tc qdisc delete dev enp1s0 parent 6666:1

    # IGC_TQAVCTRL should now be 0x0 again, but is observed as 0x9
    sudo pcimem /sys/bus/pci/devices/0000:01:00.0/resource0 0x3570 w*1

Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index dda057a3b5e3..290daa5827f0 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6053,6 +6053,7 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter)
 
 	adapter->base_time = 0;
 	adapter->cycle_time = NSEC_PER_SEC;
+	adapter->taprio_offload_enable = false;
 	adapter->qbv_config_change_errors = 0;
 
 	for (i = 0; i < adapter->num_tx_queues; i++) {
@@ -6075,8 +6076,6 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 	size_t n;
 	int i;
 
-	adapter->taprio_offload_enable = qopt->enable;
-
 	if (!qopt->enable)
 		return igc_tsn_clear_schedule(adapter);
 
@@ -6091,6 +6090,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 
 	adapter->cycle_time = qopt->cycle_time;
 	adapter->base_time = qopt->base_time;
+	adapter->taprio_offload_enable = true;
 
 	for (n = 0; n < qopt->num_entries; n++) {
 		struct tc_taprio_sched_entry *e = &qopt->entries[n];
-- 
2.39.2

[PATCH net v2 3/6] igc: Handle already enabled taprio offload for basetime 0

[Florian Kauer]

Since commit e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
it is possible to enable taprio offload with a basetime of 0.
However, the check if taprio offload is already enabled for i225
(and thus -EALREADY should be returned for igc_save_qbv_schedule)
still relied on adapter->base_time > 0.

This can be reproduced as follows:

    # TAPRIO offload (flags == 0x2) and base-time = 0
    sudo tc qdisc replace dev enp1s0 parent root handle 100 stab overhead 24 taprio \
	    num_tc 1 \
	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
	    queues 1@0 \
	    base-time 0 \
	    sched-entry S 01 300000 \
	    flags 0x2

    # The second call should fail with "Error: Device failed to setup taprio offload."
    # But that only happens if base-time was != 0
    sudo tc qdisc replace dev enp1s0 parent root handle 100 stab overhead 24 taprio \
	    num_tc 1 \
	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
	    queues 1@0 \
	    base-time 0 \
	    sched-entry S 01 300000 \
	    flags 0x2

Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 290daa5827f0..9c04df900b59 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6082,7 +6082,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 	if (qopt->base_time < 0)
 		return -ERANGE;
 
-	if (igc_is_device_id_i225(hw) && adapter->base_time)
+	if (igc_is_device_id_i225(hw) && adapter->taprio_offload_enable)
 		return -EALREADY;
 
 	if (!validate_schedule(adapter, qopt))
-- 
2.39.2

[PATCH net v2 4/6] igc: No strict mode in pure launchtime/CBS offload

[Florian Kauer]

The flags IGC_TXQCTL_STRICT_CYCLE and IGC_TXQCTL_STRICT_END
prevent the packet transmission over slot and cycle boundaries.
This is important for taprio offload where the slots and
cycles correspond to the slots and cycles configured for the
network.

However, the Qbv offload feature of the i225 is also used for
enabling TX launchtime / ETF offload. In that case, however,
the cycle has no meaning for the network and is only used
internally to adapt the base time register after a second has
passed.

Enabling strict mode in this case would unneccesarily prevent
the transmission of certain packets (i.e. at the boundary of a
second) and thus interfers with the ETF qdisc that promises
transmission at a certain point in time.

Similar to ETF, this also applies to CBS offload that also should
not be influenced by strict mode unless taprio offload would be
enabled at the same time.

This fully reverts
commit d8f45be01dd9 ("igc: Use strict cycles for Qbv scheduling")
but its commit message only describes what was already implemented
before that commit. The difference to a plain revert of that commit
is that it now copes with the base_time = 0 case that was fixed with
commit e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")

In particular, enabling strict mode leads to TX hang situations
under high traffic if taprio is applied WITHOUT taprio offload
but WITH ETF offload, e.g. as in

    sudo tc qdisc replace dev enp1s0 parent root handle 100 taprio \
	    num_tc 1 \
	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
	    queues 1@0 \
	    base-time 0 \
	    sched-entry S 01 300000 \
	    flags 0x1 \
	    txtime-delay 500000 \
	    clockid CLOCK_TAI
    sudo tc qdisc replace dev enp1s0 parent 100:1 etf \
	    clockid CLOCK_TAI \
	    delta 500000 \
	    offload \
	    skip_sock_check

and traffic generator

    sudo trafgen -i traffic.cfg -o enp1s0 --cpp -n0 -q -t1400ns

with traffic.cfg

    #define ETH_P_IP        0x0800

    {
      /* Ethernet Header */
      0x30, 0x1f, 0x9a, 0xd0, 0xf0, 0x0e,  # MAC Dest - adapt as needed
      0x24, 0x5e, 0xbe, 0x57, 0x2e, 0x36,  # MAC Src  - adapt as needed
      const16(ETH_P_IP),

      /* IPv4 Header */
      0b01000101, 0,   # IPv4 version, IHL, TOS
      const16(1028),   # IPv4 total length (UDP length + 20 bytes (IP header))
      const16(2),      # IPv4 ident
      0b01000000, 0,   # IPv4 flags, fragmentation off
      64,              # IPv4 TTL
      17,              # Protocol UDP
      csumip(14, 33),  # IPv4 checksum

      /* UDP Header */
      10,  0, 48, 1,   # IP Src - adapt as needed
      10,  0, 48, 10,  # IP Dest - adapt as needed
      const16(5555),   # UDP Src Port
      const16(6666),   # UDP Dest Port
      const16(1008),   # UDP length (UDP header 8 bytes + payload length)
      csumudp(14, 34), # UDP checksum

      /* Payload */
      fill('W', 1000),
    }

and the observed message with that is for example

 igc 0000:01:00.0 enp1s0: Detected Tx Unit Hang
   Tx Queue             <0>
   TDH                  <d0>
   TDT                  <f0>
   next_to_use          <f0>
   next_to_clean        <d0>
 buffer_info[next_to_clean]
   time_stamp           <ffff661f>
   next_to_watch        <00000000245a4efb>
   jiffies              <ffff6e48>
   desc.status          <1048000>

Fixes: d8f45be01dd9 ("igc: Use strict cycles for Qbv scheduling")
Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igc/igc_tsn.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index c6636a7264d5..63d410e7b876 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -133,8 +133,28 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter)
 		wr32(IGC_STQT(i), ring->start_time);
 		wr32(IGC_ENDQT(i), ring->end_time);
 
-		txqctl |= IGC_TXQCTL_STRICT_CYCLE |
-			IGC_TXQCTL_STRICT_END;
+		if (adapter->taprio_offload_enable) {
+			/* If taprio_offload_enable is set we are in "taprio"
+			 * mode and we need to be strict about the
+			 * cycles: only transmit a packet if it can be
+			 * completed during that cycle.
+			 *
+			 * If taprio_offload_enable is NOT true when
+			 * enabling TSN offload, the cycle should have
+			 * no external effects, but is only used internally
+			 * to adapt the base time register after a second
+			 * has passed.
+			 *
+			 * Enabling strict mode in this case would
+			 * unneccesarily prevent the transmission of
+			 * certain packets (i.e. at the boundary of a
+			 * second) and thus interfer with the launchtime
+			 * feature that promises transmission at a
+			 * certain point in time.
+			 */
+			txqctl |= IGC_TXQCTL_STRICT_CYCLE |
+				IGC_TXQCTL_STRICT_END;
+		}
 
 		if (ring->launchtime_enable)
 			txqctl |= IGC_TXQCTL_QUEUE_MODE_LAUNCHT;
-- 
2.39.2

[PATCH net v2 5/6] igc: Fix launchtime before start of cycle

[Florian Kauer]

It is possible (verified on a running system) that frames are processed
by igc_tx_launchtime with a txtime before the start of the cycle
(baset_est).

However, the result of txtime - baset_est is written into a u32,
leading to a wrap around to a positive number. The following
launchtime > 0 check will only branch to executing launchtime = 0
if launchtime is already 0.

Fix it by using a s32 before checking launchtime > 0.

Fixes: db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 9c04df900b59..565c72bd737d 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1017,7 +1017,7 @@ static __le32 igc_tx_launchtime(struct igc_ring *ring, ktime_t txtime,
 	ktime_t base_time = adapter->base_time;
 	ktime_t now = ktime_get_clocktai();
 	ktime_t baset_est, end_of_cycle;
-	u32 launchtime;
+	s32 launchtime;
 	s64 n;
 
 	n = div64_s64(ktime_sub_ns(now, base_time), cycle_time);
-- 
2.39.2

[PATCH net v2 6/6] igc: Fix inserting of empty frame for launchtime

[Florian Kauer]

The insertion of an empty frame was introduced with
commit db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
in order to ensure that the current cycle has at least one packet if
there is some packet to be scheduled for the next cycle.

However, the current implementation does not properly check if
a packet is already scheduled for the current cycle. Currently,
an empty packet is always inserted if and only if
txtime >= end_of_cycle && txtime > last_tx_cycle
but since last_tx_cycle is always either the end of the current
cycle (end_of_cycle) or the end of a previous cycle, the
second part (txtime > last_tx_cycle) is always true unless
txtime == last_tx_cycle.

What actually needs to be checked here is if the last_tx_cycle
was already written within the current cycle, so an empty frame
should only be inserted if and only if
txtime >= end_of_cycle && end_of_cycle > last_tx_cycle.

This patch does not only avoid an unnecessary insertion, but it
can actually be harmful to insert an empty packet if packets
are already scheduled in the current cycle, because it can lead
to a situation where the empty packet is actually processed
as the first packet in the upcoming cycle shifting the packet
with the first_flag even one cycle into the future, finally leading
to a TX hang.

The TX hang can be reproduced on a i225 with:

    sudo tc qdisc replace dev enp1s0 parent root handle 100 taprio \
	    num_tc 1 \
	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
	    queues 1@0 \
	    base-time 0 \
	    sched-entry S 01 300000 \
	    flags 0x1 \
	    txtime-delay 500000 \
	    clockid CLOCK_TAI
    sudo tc qdisc replace dev enp1s0 parent 100:1 etf \
	    clockid CLOCK_TAI \
	    delta 500000 \
	    offload \
	    skip_sock_check

and traffic generator

    sudo trafgen -i traffic.cfg -o enp1s0 --cpp -n0 -q -t1400ns

with traffic.cfg

    #define ETH_P_IP        0x0800

    {
      /* Ethernet Header */
      0x30, 0x1f, 0x9a, 0xd0, 0xf0, 0x0e,  # MAC Dest - adapt as needed
      0x24, 0x5e, 0xbe, 0x57, 0x2e, 0x36,  # MAC Src  - adapt as needed
      const16(ETH_P_IP),

      /* IPv4 Header */
      0b01000101, 0,   # IPv4 version, IHL, TOS
      const16(1028),   # IPv4 total length (UDP length + 20 bytes (IP header))
      const16(2),      # IPv4 ident
      0b01000000, 0,   # IPv4 flags, fragmentation off
      64,              # IPv4 TTL
      17,              # Protocol UDP
      csumip(14, 33),  # IPv4 checksum

      /* UDP Header */
      10,  0, 48, 1,   # IP Src - adapt as needed
      10,  0, 48, 10,  # IP Dest - adapt as needed
      const16(5555),   # UDP Src Port
      const16(6666),   # UDP Dest Port
      const16(1008),   # UDP length (UDP header 8 bytes + payload length)
      csumudp(14, 34), # UDP checksum

      /* Payload */
      fill('W', 1000),
    }

and the observed message with that is for example

 igc 0000:01:00.0 enp1s0: Detected Tx Unit Hang
   Tx Queue             <0>
   TDH                  <32>
   TDT                  <3c>
   next_to_use          <3c>
   next_to_clean        <32>
 buffer_info[next_to_clean]
   time_stamp           <ffff26a8>
   next_to_watch        <00000000632a1828>
   jiffies              <ffff27f8>
   desc.status          <1048000>

Fixes: db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 565c72bd737d..f847c9a408d6 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1030,7 +1030,7 @@ static __le32 igc_tx_launchtime(struct igc_ring *ring, ktime_t txtime,
 			*first_flag = true;
 			ring->last_ff_cycle = baset_est;
 
-			if (ktime_compare(txtime, ring->last_tx_cycle) > 0)
+			if (ktime_compare(end_of_cycle, ring->last_tx_cycle) > 0)
 				*insert_empty = true;
 		}
 	}
-- 
2.39.2

Re: [PATCH net v2 0/6] igc: Fix corner cases for TSN offload

[Vinicius Costa Gomes]

Florian Kauer <florian.kauer@linutronix.de> writes:

> The igc driver supports several different offloading capabilities
> relevant in the TSN context. Recent patches in this area introduced
> regressions for certain corner cases that are fixed in this series.
>
> Each of the patches (except the first one) addresses a different
> regression that can be separately reproduced. Still, they have
> overlapping code changes so they should not be separately applied.
>
> Especially #4 and #6 address the same observation,
> but both need to be applied to avoid TX hang occurrences in
> the scenario described in the patches.
>
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
>
> ---

For the series:

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>


Cheers,
-- 
Vinicius

RE: [PATCH net v2 0/6] igc: Fix corner cases for TSN offload

[Zulkifli, Muhammad Husaini]

> -----Original Message-----
> From: Florian Kauer <florian.kauer@linutronix.de>
> Sent: Monday, 19 June, 2023 6:09 PM
> To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Gomes, Vinicius
> <vinicius.gomes@intel.com>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Tan Tee Min
> <tee.min.tan@linux.intel.com>; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@intel.com>; Gunasekaran, Aravindhan
> <aravindhan.gunasekaran@intel.com>; Chilakala, Mallikarjuna
> <mallikarjuna.chilakala@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; kurt@linutronix.de; florian.kauer@linutronix.de
> Subject: [PATCH net v2 0/6] igc: Fix corner cases for TSN offload
> 
> The igc driver supports several different offloading capabilities relevant in the
> TSN context. Recent patches in this area introduced regressions for certain
> corner cases that are fixed in this series.
> 
> Each of the patches (except the first one) addresses a different regression
> that can be separately reproduced. Still, they have overlapping code changes
> so they should not be separately applied.
> 
> Especially #4 and #6 address the same observation, but both need to be
> applied to avoid TX hang occurrences in the scenario described in the
> patches.
> 
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> 
> ---
> 
> v2: - Rebased onto net. #1-#2 needed adaptations, others unmodified.
>     - Extend #3 commit message that it only regards i225.
> 
> ---
> 
> Florian Kauer (6):
>   igc: Rename qbv_enable to taprio_offload_enable
>   igc: Do not enable taprio offload for invalid arguments
>   igc: Handle already enabled taprio offload for basetime 0
>   igc: No strict mode in pure launchtime/CBS offload
>   igc: Fix launchtime before start of cycle
>   igc: Fix inserting of empty frame for launchtime

Thanks!

Reviewed-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

> 
>  drivers/net/ethernet/intel/igc/igc.h      |  2 +-
>  drivers/net/ethernet/intel/igc/igc_main.c | 10 ++++-----
> drivers/net/ethernet/intel/igc/igc_tsn.c  | 26 ++++++++++++++++++++---
>  3 files changed, 29 insertions(+), 9 deletions(-)
> 
> --
> 2.39.2

Re: [Intel-wired-lan] [PATCH net v2 1/6] igc: Rename qbv_enable to taprio_offload_enable

[naamax.meir]

On 6/19/2023 13:08, Florian Kauer wrote:
> In the current implementation the flags adapter->qbv_enable
> and IGC_FLAG_TSN_QBV_ENABLED have a similar name, but do not
> have the same meaning. The first one is used only to indicate
> taprio offload (i.e. when igc_save_qbv_schedule was called),
> while the second one corresponds to the Qbv mode of the hardware.
> However, the second one is also used to support the TX launchtime
> feature, i.e. ETF qdisc offload. This leads to situations where
> adapter->qbv_enable is false, but the flag IGC_FLAG_TSN_QBV_ENABLED
> is set. This is prone to confusion.
> 
> The rename should reduce this confusion. Since it is a pure
> rename, it has no impact on functionality.
> 
> Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   drivers/net/ethernet/intel/igc/igc.h      | 2 +-
>   drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>   drivers/net/ethernet/intel/igc/igc_tsn.c  | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)

Tested-by: Naama Meir <naamax.meir@linux.intel.com>

Re: [PATCH net v2 1/6] igc: Rename qbv_enable to taprio_offload_enable

[Florian Kauer]

Hi Markus,

On 02.07.23 18:55, Markus Elfring wrote:
>> The rename should reduce this confusion.
> 
> Would the wording “Reduce this confusion by renaming a variable at three places”
> be more appropriate for a subsequent change description?
> 
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94

Thanks for pointing that out (also in your other mail for this series).
I will be more careful regarding the use of imperative mood.

> 
>>                                          Since it is a pure
>> rename, it has no impact on functionality.
>>
>> Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
> 
> How does such information fit together?

The referenced commit introduced an issue into the kernel by
introducing a variable that does not exactly describe its actual purpose.
It is not only a cosmetic change, but in my view this confusion
was related to other issues (see the other patches). So, it seemed to be worth
fixing alongside with the other fixes, even if it does not directly impact
functionality if it is applied or not (until someone else comes along,
also gets confused and introduces another bug...).

Thanks,
Florian

Re: [Intel-wired-lan] [PATCH net v2 2/6] igc: Do not enable taprio offload for invalid arguments

[naamax.meir]

On 6/19/2023 13:08, Florian Kauer wrote:
> Only set adapter->taprio_offload_enable after validating the arguments.
> Otherwise, it stays set even if the offload was not enabled.
> Since the subsequent code does not get executed in case of invalid
> arguments, it will not be read at first.
> However, by activating and then deactivating another offload
> (e.g. ETF/TX launchtime offload), taprio_offload_enable is read
> and erroneously keeps the offload feature of the NIC enabled.
> 
> This can be reproduced as follows:
> 
>      # TAPRIO offload (flags == 0x2) and negative base-time leading to expected -ERANGE
>      sudo tc qdisc replace dev enp1s0 parent root handle 100 stab overhead 24 taprio \
> 	    num_tc 1 \
> 	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
> 	    queues 1@0 \
> 	    base-time -1000 \
> 	    sched-entry S 01 300000 \
> 	    flags 0x2
> 
>      # IGC_TQAVCTRL is 0x0 as expected (iomem=relaxed for reading register)
>      sudo pcimem /sys/bus/pci/devices/0000:01:00.0/resource0 0x3570 w*1
> 
>      # Activate ETF offload
>      sudo tc qdisc replace dev enp1s0 parent root handle 6666 mqprio \
> 	    num_tc 3 \
> 	    map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \
> 	    queues 1@0 1@1 2@2 \
> 	    hw 0
>      sudo tc qdisc add dev enp1s0 parent 6666:1 etf \
> 	    clockid CLOCK_TAI \
> 	    delta 500000 \
> 	    offload
> 
>      # IGC_TQAVCTRL is 0x9 as expected
>      sudo pcimem /sys/bus/pci/devices/0000:01:00.0/resource0 0x3570 w*1
> 
>      # Deactivate ETF offload again
>      sudo tc qdisc delete dev enp1s0 parent 6666:1
> 
>      # IGC_TQAVCTRL should now be 0x0 again, but is observed as 0x9
>      sudo pcimem /sys/bus/pci/devices/0000:01:00.0/resource0 0x3570 w*1
> 
> Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)


Tested-by: Naama Meir <naamax.meir@linux.intel.com>

Re: [Intel-wired-lan] [PATCH net v2 3/6] igc: Handle already enabled taprio offload for basetime 0

[naamax.meir]

On 6/19/2023 13:08, Florian Kauer wrote:
> Since commit e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
> it is possible to enable taprio offload with a basetime of 0.
> However, the check if taprio offload is already enabled for i225
> (and thus -EALREADY should be returned for igc_save_qbv_schedule)
> still relied on adapter->base_time > 0.
> 
> This can be reproduced as follows:
> 
>      # TAPRIO offload (flags == 0x2) and base-time = 0
>      sudo tc qdisc replace dev enp1s0 parent root handle 100 stab overhead 24 taprio \
> 	    num_tc 1 \
> 	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
> 	    queues 1@0 \
> 	    base-time 0 \
> 	    sched-entry S 01 300000 \
> 	    flags 0x2
> 
>      # The second call should fail with "Error: Device failed to setup taprio offload."
>      # But that only happens if base-time was != 0
>      sudo tc qdisc replace dev enp1s0 parent root handle 100 stab overhead 24 taprio \
> 	    num_tc 1 \
> 	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
> 	    queues 1@0 \
> 	    base-time 0 \
> 	    sched-entry S 01 300000 \
> 	    flags 0x2
> 
> Fixes: e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Tested-by: Naama Meir <naamax.meir@linux.intel.com>

Re: [Intel-wired-lan] [PATCH net v2 4/6] igc: No strict mode in pure launchtime/CBS offload

[naamax.meir]

On 6/19/2023 13:08, Florian Kauer wrote:
> The flags IGC_TXQCTL_STRICT_CYCLE and IGC_TXQCTL_STRICT_END
> prevent the packet transmission over slot and cycle boundaries.
> This is important for taprio offload where the slots and
> cycles correspond to the slots and cycles configured for the
> network.
> 
> However, the Qbv offload feature of the i225 is also used for
> enabling TX launchtime / ETF offload. In that case, however,
> the cycle has no meaning for the network and is only used
> internally to adapt the base time register after a second has
> passed.
> 
> Enabling strict mode in this case would unneccesarily prevent
> the transmission of certain packets (i.e. at the boundary of a
> second) and thus interfers with the ETF qdisc that promises
> transmission at a certain point in time.
> 
> Similar to ETF, this also applies to CBS offload that also should
> not be influenced by strict mode unless taprio offload would be
> enabled at the same time.
> 
> This fully reverts
> commit d8f45be01dd9 ("igc: Use strict cycles for Qbv scheduling")
> but its commit message only describes what was already implemented
> before that commit. The difference to a plain revert of that commit
> is that it now copes with the base_time = 0 case that was fixed with
> commit e17090eb2494 ("igc: allow BaseTime 0 enrollment for Qbv")
> 
> In particular, enabling strict mode leads to TX hang situations
> under high traffic if taprio is applied WITHOUT taprio offload
> but WITH ETF offload, e.g. as in
> 
>      sudo tc qdisc replace dev enp1s0 parent root handle 100 taprio \
> 	    num_tc 1 \
> 	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
> 	    queues 1@0 \
> 	    base-time 0 \
> 	    sched-entry S 01 300000 \
> 	    flags 0x1 \
> 	    txtime-delay 500000 \
> 	    clockid CLOCK_TAI
>      sudo tc qdisc replace dev enp1s0 parent 100:1 etf \
> 	    clockid CLOCK_TAI \
> 	    delta 500000 \
> 	    offload \
> 	    skip_sock_check
> 
> and traffic generator
> 
>      sudo trafgen -i traffic.cfg -o enp1s0 --cpp -n0 -q -t1400ns
> 
> with traffic.cfg
> 
>      #define ETH_P_IP        0x0800
> 
>      {
>        /* Ethernet Header */
>        0x30, 0x1f, 0x9a, 0xd0, 0xf0, 0x0e,  # MAC Dest - adapt as needed
>        0x24, 0x5e, 0xbe, 0x57, 0x2e, 0x36,  # MAC Src  - adapt as needed
>        const16(ETH_P_IP),
> 
>        /* IPv4 Header */
>        0b01000101, 0,   # IPv4 version, IHL, TOS
>        const16(1028),   # IPv4 total length (UDP length + 20 bytes (IP header))
>        const16(2),      # IPv4 ident
>        0b01000000, 0,   # IPv4 flags, fragmentation off
>        64,              # IPv4 TTL
>        17,              # Protocol UDP
>        csumip(14, 33),  # IPv4 checksum
> 
>        /* UDP Header */
>        10,  0, 48, 1,   # IP Src - adapt as needed
>        10,  0, 48, 10,  # IP Dest - adapt as needed
>        const16(5555),   # UDP Src Port
>        const16(6666),   # UDP Dest Port
>        const16(1008),   # UDP length (UDP header 8 bytes + payload length)
>        csumudp(14, 34), # UDP checksum
> 
>        /* Payload */
>        fill('W', 1000),
>      }
> 
> and the observed message with that is for example
> 
>   igc 0000:01:00.0 enp1s0: Detected Tx Unit Hang
>     Tx Queue             <0>
>     TDH                  <d0>
>     TDT                  <f0>
>     next_to_use          <f0>
>     next_to_clean        <d0>
>   buffer_info[next_to_clean]
>     time_stamp           <ffff661f>
>     next_to_watch        <00000000245a4efb>
>     jiffies              <ffff6e48>
>     desc.status          <1048000>
> 
> Fixes: d8f45be01dd9 ("igc: Use strict cycles for Qbv scheduling")
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   drivers/net/ethernet/intel/igc/igc_tsn.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)

Tested-by: Naama Meir <naamax.meir@linux.intel.com>

Re: [Intel-wired-lan] [PATCH net v2 4/6] igc: No strict mode in pure launchtime/CBS offload

[Paul Menzel]

Dear Florian,


Thank you for your patch.

Am 19.06.23 um 12:08 schrieb Florian Kauer:

[…]

For the commit summary/title, maybe make it a statement by using a verb 
(in imperative mood)? Maybe:

> Forbid strict mode in pure launchtime/CBS offload


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH net v2 5/6] igc: Fix launchtime before start of cycle

[naamax.meir]

On 6/19/2023 13:08, Florian Kauer wrote:
> It is possible (verified on a running system) that frames are processed
> by igc_tx_launchtime with a txtime before the start of the cycle
> (baset_est).
> 
> However, the result of txtime - baset_est is written into a u32,
> leading to a wrap around to a positive number. The following
> launchtime > 0 check will only branch to executing launchtime = 0
> if launchtime is already 0.
> 
> Fix it by using a s32 before checking launchtime > 0.
> 
> Fixes: db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)


Tested-by: Naama Meir <naamax.meir@linux.intel.com>

Re: [Intel-wired-lan] [PATCH net v2 6/6] igc: Fix inserting of empty frame for launchtime

[naamax.meir]

On 6/19/2023 13:08, Florian Kauer wrote:
> The insertion of an empty frame was introduced with
> commit db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
> in order to ensure that the current cycle has at least one packet if
> there is some packet to be scheduled for the next cycle.
> 
> However, the current implementation does not properly check if
> a packet is already scheduled for the current cycle. Currently,
> an empty packet is always inserted if and only if
> txtime >= end_of_cycle && txtime > last_tx_cycle
> but since last_tx_cycle is always either the end of the current
> cycle (end_of_cycle) or the end of a previous cycle, the
> second part (txtime > last_tx_cycle) is always true unless
> txtime == last_tx_cycle.
> 
> What actually needs to be checked here is if the last_tx_cycle
> was already written within the current cycle, so an empty frame
> should only be inserted if and only if
> txtime >= end_of_cycle && end_of_cycle > last_tx_cycle.
> 
> This patch does not only avoid an unnecessary insertion, but it
> can actually be harmful to insert an empty packet if packets
> are already scheduled in the current cycle, because it can lead
> to a situation where the empty packet is actually processed
> as the first packet in the upcoming cycle shifting the packet
> with the first_flag even one cycle into the future, finally leading
> to a TX hang.
> 
> The TX hang can be reproduced on a i225 with:
> 
>      sudo tc qdisc replace dev enp1s0 parent root handle 100 taprio \
> 	    num_tc 1 \
> 	    map 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 \
> 	    queues 1@0 \
> 	    base-time 0 \
> 	    sched-entry S 01 300000 \
> 	    flags 0x1 \
> 	    txtime-delay 500000 \
> 	    clockid CLOCK_TAI
>      sudo tc qdisc replace dev enp1s0 parent 100:1 etf \
> 	    clockid CLOCK_TAI \
> 	    delta 500000 \
> 	    offload \
> 	    skip_sock_check
> 
> and traffic generator
> 
>      sudo trafgen -i traffic.cfg -o enp1s0 --cpp -n0 -q -t1400ns
> 
> with traffic.cfg
> 
>      #define ETH_P_IP        0x0800
> 
>      {
>        /* Ethernet Header */
>        0x30, 0x1f, 0x9a, 0xd0, 0xf0, 0x0e,  # MAC Dest - adapt as needed
>        0x24, 0x5e, 0xbe, 0x57, 0x2e, 0x36,  # MAC Src  - adapt as needed
>        const16(ETH_P_IP),
> 
>        /* IPv4 Header */
>        0b01000101, 0,   # IPv4 version, IHL, TOS
>        const16(1028),   # IPv4 total length (UDP length + 20 bytes (IP header))
>        const16(2),      # IPv4 ident
>        0b01000000, 0,   # IPv4 flags, fragmentation off
>        64,              # IPv4 TTL
>        17,              # Protocol UDP
>        csumip(14, 33),  # IPv4 checksum
> 
>        /* UDP Header */
>        10,  0, 48, 1,   # IP Src - adapt as needed
>        10,  0, 48, 10,  # IP Dest - adapt as needed
>        const16(5555),   # UDP Src Port
>        const16(6666),   # UDP Dest Port
>        const16(1008),   # UDP length (UDP header 8 bytes + payload length)
>        csumudp(14, 34), # UDP checksum
> 
>        /* Payload */
>        fill('W', 1000),
>      }
> 
> and the observed message with that is for example
> 
>   igc 0000:01:00.0 enp1s0: Detected Tx Unit Hang
>     Tx Queue             <0>
>     TDH                  <32>
>     TDT                  <3c>
>     next_to_use          <3c>
>     next_to_clean        <32>
>   buffer_info[next_to_clean]
>     time_stamp           <ffff26a8>
>     next_to_watch        <00000000632a1828>
>     jiffies              <ffff27f8>
>     desc.status          <1048000>
> 
> Fixes: db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Tested-by: Naama Meir <naamax.meir@linux.intel.com>