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

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

[Tony Nguyen]

Florian Kauer says:

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>
Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Reviewed-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

The following are changes since commit 8139dccd464aaee4a2c351506ff883733c6ca5a3:
  udp6: add a missing call into udp_fail_queue_rcv_skb tracepoint
and are available in the git repository at:
This series contains updates to

The following are changes since commit 9d0aba98316d00f9c0a4506fc15f5ed9241bc1fd:
  gve: unify driver name usage
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 1GbE

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 | 24 ++++++++-------------
 drivers/net/ethernet/intel/igc/igc_tsn.c  | 26 ++++++++++++++++++++---
 3 files changed, 33 insertions(+), 19 deletions(-)

-- 
2.38.1

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

[Tony Nguyen]

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

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>
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      | 2 +-
 drivers/net/ethernet/intel/igc/igc_main.c | 6 +++---
 drivers/net/ethernet/intel/igc/igc_tsn.c  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 639a50c02537..9db384f66a8e 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -191,7 +191,7 @@ struct igc_adapter {
 	int tc_setup_type;
 	ktime_t base_time;
 	ktime_t cycle_time;
-	bool qbv_enable;
+	bool taprio_offload_enable;
 	u32 qbv_config_change_errors;
 	bool qbv_transition;
 	unsigned int qbv_count;
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 281a0e35b9d1..fae534ef1c4f 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6126,16 +6126,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 
 	switch (qopt->cmd) {
 	case TAPRIO_CMD_REPLACE:
-		adapter->qbv_enable = true;
+		adapter->taprio_offload_enable = true;
 		break;
 	case TAPRIO_CMD_DESTROY:
-		adapter->qbv_enable = false;
+		adapter->taprio_offload_enable = false;
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
-	if (!adapter->qbv_enable)
+	if (!adapter->taprio_offload_enable)
 		return igc_tsn_clear_schedule(adapter);
 
 	if (qopt->base_time < 0)
diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
index 3cdb0c988728..b76ebfc10b1d 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.38.1

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

[Leon Romanovsky]

On Mon, Jul 10, 2023 at 09:34:58AM -0700, Tony Nguyen wrote:
> From: Florian Kauer <florian.kauer@linutronix.de>
> 
> 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.

And shouldn't be sent to net, but to net-next.

Thanks

> 
> 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>
> 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      | 2 +-
>  drivers/net/ethernet/intel/igc/igc_main.c | 6 +++---
>  drivers/net/ethernet/intel/igc/igc_tsn.c  | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 639a50c02537..9db384f66a8e 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -191,7 +191,7 @@ struct igc_adapter {
>  	int tc_setup_type;
>  	ktime_t base_time;
>  	ktime_t cycle_time;
> -	bool qbv_enable;
> +	bool taprio_offload_enable;
>  	u32 qbv_config_change_errors;
>  	bool qbv_transition;
>  	unsigned int qbv_count;
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 281a0e35b9d1..fae534ef1c4f 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6126,16 +6126,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>  
>  	switch (qopt->cmd) {
>  	case TAPRIO_CMD_REPLACE:
> -		adapter->qbv_enable = true;
> +		adapter->taprio_offload_enable = true;
>  		break;
>  	case TAPRIO_CMD_DESTROY:
> -		adapter->qbv_enable = false;
> +		adapter->taprio_offload_enable = false;
>  		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
>  
> -	if (!adapter->qbv_enable)
> +	if (!adapter->taprio_offload_enable)
>  		return igc_tsn_clear_schedule(adapter);
>  
>  	if (qopt->base_time < 0)
> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
> index 3cdb0c988728..b76ebfc10b1d 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.38.1
> 
> 

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

[Florian Kauer]

Hi Leon,

On 11.07.23 09:01, Leon Romanovsky wrote:
> On Mon, Jul 10, 2023 at 09:34:58AM -0700, Tony Nguyen wrote:
>> From: Florian Kauer <florian.kauer@linutronix.de>
>>
>> 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.
> 
> And shouldn't be sent to net, but to net-next.> 
> Thanks

In principle I fully agree that sole renames are not intended for net.
But in this case the rename is tightly coupled with the other patches
of the series, not only due to overlapping code changes, but in particular
because the naming might very likely be one root cause of the regressions.

I probably should have just squashed it with the second patch,
but my initial idea was to keep them separate for clarity.

Also see:
https://lore.kernel.org/netdev/SJ1PR11MB6180B5C87699252B91FB7EE1B858A@SJ1PR11MB6180.namprd11.prod.outlook.com/
https://lore.kernel.org/netdev/0c02e976-0da6-8ed8-4546-4df7af4ebed5@linutronix.de/

Thanks,
Florian
 
>>
>> 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>
>> 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      | 2 +-
>>  drivers/net/ethernet/intel/igc/igc_main.c | 6 +++---
>>  drivers/net/ethernet/intel/igc/igc_tsn.c  | 2 +-
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
>> index 639a50c02537..9db384f66a8e 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -191,7 +191,7 @@ struct igc_adapter {
>>  	int tc_setup_type;
>>  	ktime_t base_time;
>>  	ktime_t cycle_time;
>> -	bool qbv_enable;
>> +	bool taprio_offload_enable;
>>  	u32 qbv_config_change_errors;
>>  	bool qbv_transition;
>>  	unsigned int qbv_count;
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 281a0e35b9d1..fae534ef1c4f 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -6126,16 +6126,16 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>>  
>>  	switch (qopt->cmd) {
>>  	case TAPRIO_CMD_REPLACE:
>> -		adapter->qbv_enable = true;
>> +		adapter->taprio_offload_enable = true;
>>  		break;
>>  	case TAPRIO_CMD_DESTROY:
>> -		adapter->qbv_enable = false;
>> +		adapter->taprio_offload_enable = false;
>>  		break;
>>  	default:
>>  		return -EOPNOTSUPP;
>>  	}
>>  
>> -	if (!adapter->qbv_enable)
>> +	if (!adapter->taprio_offload_enable)
>>  		return igc_tsn_clear_schedule(adapter);
>>  
>>  	if (qopt->base_time < 0)
>> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c
>> index 3cdb0c988728..b76ebfc10b1d 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.38.1
>>
>>

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

[Leon Romanovsky]

On Tue, Jul 11, 2023 at 09:18:31AM +0200, Florian Kauer wrote:
> Hi Leon,
> 
> On 11.07.23 09:01, Leon Romanovsky wrote:
> > On Mon, Jul 10, 2023 at 09:34:58AM -0700, Tony Nguyen wrote:
> >> From: Florian Kauer <florian.kauer@linutronix.de>
> >>
> >> 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.
> > 
> > And shouldn't be sent to net, but to net-next.> 
> > Thanks
> 
> In principle I fully agree that sole renames are not intended for net.
> But in this case the rename is tightly coupled with the other patches
> of the series, not only due to overlapping code changes, but in particular
> because the naming might very likely be one root cause of the regressions.

I understand the intention, but your second patch showed that rename was
premature.

Thanks

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

[Florian Kauer]

On 11.07.23 09:32, Leon Romanovsky wrote:
> On Tue, Jul 11, 2023 at 09:18:31AM +0200, Florian Kauer wrote:
>> Hi Leon,
>>
>> On 11.07.23 09:01, Leon Romanovsky wrote:
>>> On Mon, Jul 10, 2023 at 09:34:58AM -0700, Tony Nguyen wrote:
>>>> From: Florian Kauer <florian.kauer@linutronix.de>
>>>>
>>>> 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.
>>>
>>> And shouldn't be sent to net, but to net-next.> 
>>> Thanks
>>
>> In principle I fully agree that sole renames are not intended for net.
>> But in this case the rename is tightly coupled with the other patches
>> of the series, not only due to overlapping code changes, but in particular
>> because the naming might very likely be one root cause of the regressions.
> 
> I understand the intention, but your second patch showed that rename was
> premature.
> 
> Thanks

The second patch does not touch the rename in igc.h and igc_tsn.c...
(and the latter is from the context probably the most relevant one)
But I see what you mean. I am fine with both squashing and keeping it separate,
but I have no idea how the preferred process is since this
is already so far through the pipeline...

Thanks,
Florian

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

[Jakub Kicinski]

On Tue, 11 Jul 2023 09:51:34 +0200 Florian Kauer wrote:
> > I understand the intention, but your second patch showed that rename was
> > premature.

I think it's fine. It's a rename, it can't regress anything.
And the separate commit message clearly describing reasoning
is good to have.

> The second patch does not touch the rename in igc.h and igc_tsn.c...
> (and the latter is from the context probably the most relevant one)
> But I see what you mean. I am fine with both squashing and keeping it separate,
> but I have no idea how the preferred process is since this
> is already so far through the pipeline...

"This is so far through the pipeline" is an argument which may elicit 
a very negative reaction upstream :)

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

[Florian Kauer]

Hi Jakub,

On 12.07.23 02:58, Jakub Kicinski wrote:
> On Tue, 11 Jul 2023 09:51:34 +0200 Florian Kauer wrote:
>>> I understand the intention, but your second patch showed that rename was
>>> premature.
> 
> I think it's fine. It's a rename, it can't regress anything.
> And the separate commit message clearly describing reasoning
> is good to have.
> 
>> The second patch does not touch the rename in igc.h and igc_tsn.c...
>> (and the latter is from the context probably the most relevant one)
>> But I see what you mean. I am fine with both squashing and keeping it separate,
>> but I have no idea how the preferred process is since this
>> is already so far through the pipeline...
> 
> "This is so far through the pipeline" is an argument which may elicit 
> a very negative reaction upstream :)

Sorry, I didn't mean to use that as an argument to push it through.
It is just that since this is my first patch series (except a small
contribution some years ago), I just honestly do not know what the
usual practice in such a situation would be.

E.g. if I would just send a new version and if yes, which tags I would
keep since it would just be a squash. Especially, if it needs to be
retested. Or if Tony would directly squash it on his tree, or...

I personally would have no problem with doing extra work if it improves
the series, I just do not want to provoke unnecessary work or confusion
for others by doing it in an unusual way.

Greetings,
Florian

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

[Tony Nguyen]

On 7/11/2023 11:53 PM, Florian Kauer wrote:
> Hi Jakub,
> 
> On 12.07.23 02:58, Jakub Kicinski wrote:
>> On Tue, 11 Jul 2023 09:51:34 +0200 Florian Kauer wrote:
>>>> I understand the intention, but your second patch showed that rename was
>>>> premature.
>>
>> I think it's fine. It's a rename, it can't regress anything.
>> And the separate commit message clearly describing reasoning
>> is good to have.
>>
>>> The second patch does not touch the rename in igc.h and igc_tsn.c...
>>> (and the latter is from the context probably the most relevant one)
>>> But I see what you mean. I am fine with both squashing and keeping it separate,
>>> but I have no idea how the preferred process is since this
>>> is already so far through the pipeline...
>>
>> "This is so far through the pipeline" is an argument which may elicit
>> a very negative reaction upstream :)
> 
> Sorry, I didn't mean to use that as an argument to push it through.
> It is just that since this is my first patch series (except a small
> contribution some years ago), I just honestly do not know what the
> usual practice in such a situation would be.
> 
> E.g. if I would just send a new version and if yes, which tags I would
> keep since it would just be a squash. Especially, if it needs to be
> retested. Or if Tony would directly squash it on his tree, or...

Hi Florian,

I would expect it as a new version of your submission [1]. If anything 
needs to be done/changed on the back end for an updated PR to netdev, 
I'd take care of that.

It does look like this was pulled though so any changes will need to be 
follow-on patches.

Thanks,
Tony

> I personally would have no problem with doing extra work if it improves
> the series, I just do not want to provoke unnecessary work or confusion
> for others by doing it in an unusual way.
> 
> Greetings,
> Florian

[1] 
https://lore.kernel.org/netdev/20230619100858.116286-1-florian.kauer@linutronix.de/

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

[Tony Nguyen]

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

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>
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 | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index fae534ef1c4f..fb8e55c7c402 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6097,6 +6097,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;
 	adapter->qbv_transition = false;
 	adapter->qbv_count = 0;
@@ -6124,20 +6125,12 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
 	size_t n;
 	int i;
 
-	switch (qopt->cmd) {
-	case TAPRIO_CMD_REPLACE:
-		adapter->taprio_offload_enable = true;
-		break;
-	case TAPRIO_CMD_DESTROY:
-		adapter->taprio_offload_enable = false;
-		break;
-	default:
-		return -EOPNOTSUPP;
-	}
-
-	if (!adapter->taprio_offload_enable)
+	if (qopt->cmd == TAPRIO_CMD_DESTROY)
 		return igc_tsn_clear_schedule(adapter);
 
+	if (qopt->cmd != TAPRIO_CMD_REPLACE)
+		return -EOPNOTSUPP;
+
 	if (qopt->base_time < 0)
 		return -ERANGE;
 
@@ -6149,6 +6142,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;
 
 	igc_ptp_read(adapter, &now);
 
-- 
2.38.1

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

[Leon Romanovsky]

On Mon, Jul 10, 2023 at 09:34:59AM -0700, Tony Nguyen wrote:
> From: Florian Kauer <florian.kauer@linutronix.de>
> 
> 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>
> 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 | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index fae534ef1c4f..fb8e55c7c402 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6097,6 +6097,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;
>  	adapter->qbv_transition = false;
>  	adapter->qbv_count = 0;
> @@ -6124,20 +6125,12 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter,
>  	size_t n;
>  	int i;
>  
> -	switch (qopt->cmd) {
> -	case TAPRIO_CMD_REPLACE:
> -		adapter->taprio_offload_enable = true;
> -		break;
> -	case TAPRIO_CMD_DESTROY:
> -		adapter->taprio_offload_enable = false;
> -		break;
> -	default:
> -		return -EOPNOTSUPP;
> -	}

All this deleted code was changed in the previous patch. It raises the
question again if previous patch was really needed to be sent to net.

Thanks

> -
> -	if (!adapter->taprio_offload_enable)
> +	if (qopt->cmd == TAPRIO_CMD_DESTROY)
>  		return igc_tsn_clear_schedule(adapter);
>  
> +	if (qopt->cmd != TAPRIO_CMD_REPLACE)
> +		return -EOPNOTSUPP;
> +
>  	if (qopt->base_time < 0)
>  		return -ERANGE;
>  
> @@ -6149,6 +6142,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;
>  
>  	igc_ptp_read(adapter, &now);
>  
> -- 
> 2.38.1
> 
> 

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

[Tony Nguyen]

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

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 (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>
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 | 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 fb8e55c7c402..5d24930fed8f 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6134,7 +6134,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.38.1

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

[Leon Romanovsky]

On Mon, Jul 10, 2023 at 09:35:00AM -0700, Tony Nguyen wrote:
> From: Florian Kauer <florian.kauer@linutronix.de>
> 
> 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 (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>
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

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

[Tony Nguyen]

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

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 unnecessarily prevent
the transmission of certain packets (i.e. at the boundary of a
second) and thus interferes 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>
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_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 b76ebfc10b1d..a9c08321aca9 100644
--- a/drivers/net/ethernet/intel/igc/igc_tsn.c
+++ b/drivers/net/ethernet/intel/igc/igc_tsn.c
@@ -132,8 +132,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
+			 * unnecessarily prevent the transmission of
+			 * certain packets (i.e. at the boundary of a
+			 * second) and thus interfere 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.38.1

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

[Leon Romanovsky]

On Mon, Jul 10, 2023 at 09:35:01AM -0700, Tony Nguyen wrote:
> From: Florian Kauer <florian.kauer@linutronix.de>
> 
> 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 unnecessarily prevent
> the transmission of certain packets (i.e. at the boundary of a
> second) and thus interferes 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>
> 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_tsn.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

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

[Tony Nguyen]

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

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>
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 | 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 5d24930fed8f..4855caa3bae4 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1016,7 +1016,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.38.1

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

[Leon Romanovsky]

On Mon, Jul 10, 2023 at 09:35:02AM -0700, Tony Nguyen wrote:
> From: Florian Kauer <florian.kauer@linutronix.de>
> 
> 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>
> 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 | 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 5d24930fed8f..4855caa3bae4 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1016,7 +1016,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;

The rest of igc_tx_launchtime() function is very questionable,
as ktime_sub_ns() returns ktime_t which is s64.

  1049         launchtime = ktime_sub_ns(txtime, baset_est);
  1050         if (launchtime > 0)
  1051                 div_s64_rem(launchtime, cycle_time, &launchtime);
  1052         else
  1053                 launchtime = 0;
  1054
  1055         return cpu_to_le32(launchtime);


>  	s64 n;
>  
>  	n = div64_s64(ktime_sub_ns(now, base_time), cycle_time);
> -- 
> 2.38.1
> 
> 

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

[Florian Kauer]

On 11.07.23 09:09, Leon Romanovsky wrote:
> On Mon, Jul 10, 2023 at 09:35:02AM -0700, Tony Nguyen wrote:
>> From: Florian Kauer <florian.kauer@linutronix.de>
>>
>> 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>
>> 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 | 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 5d24930fed8f..4855caa3bae4 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -1016,7 +1016,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;
> 
> The rest of igc_tx_launchtime() function is very questionable,
> as ktime_sub_ns() returns ktime_t which is s64.
> 
>   1049         launchtime = ktime_sub_ns(txtime, baset_est);
>   1050         if (launchtime > 0)
>   1051                 div_s64_rem(launchtime, cycle_time, &launchtime);
>   1052         else
>   1053                 launchtime = 0;
>   1054
>   1055         return cpu_to_le32(launchtime);
> 

If I understand correctly, ktime_sub_ns(txtime, baset_est) will only return
something larger than s32 max if cycle_time is larger than s32 max and if that
is the case everything will be broken anyway since the corresponding hardware
register only holds 30 bits.

However, I do not see on first inspection where that case is properly handled
(probably just by rejecting the TAPRIO schedule).

Can someone with more experience in that area please jump in?

Thanks,
Florian

> 
>>  	s64 n;
>>  
>>  	n = div64_s64(ktime_sub_ns(now, base_time), cycle_time);
>> -- 
>> 2.38.1
>>
>>

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

[Leon Romanovsky]

On Tue, Jul 11, 2023 at 10:37:48AM +0200, Florian Kauer wrote:
> On 11.07.23 09:09, Leon Romanovsky wrote:
> > On Mon, Jul 10, 2023 at 09:35:02AM -0700, Tony Nguyen wrote:
> >> From: Florian Kauer <florian.kauer@linutronix.de>
> >>
> >> 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>
> >> 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 | 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 5d24930fed8f..4855caa3bae4 100644
> >> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> >> @@ -1016,7 +1016,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;
> > 
> > The rest of igc_tx_launchtime() function is very questionable,
> > as ktime_sub_ns() returns ktime_t which is s64.
> > 
> >   1049         launchtime = ktime_sub_ns(txtime, baset_est);
> >   1050         if (launchtime > 0)
> >   1051                 div_s64_rem(launchtime, cycle_time, &launchtime);
> >   1052         else
> >   1053                 launchtime = 0;
> >   1054
> >   1055         return cpu_to_le32(launchtime);
> > 
> 
> If I understand correctly, ktime_sub_ns(txtime, baset_est) will only return
> something larger than s32 max if cycle_time is larger than s32 max and if that
> is the case everything will be broken anyway since the corresponding hardware
> register only holds 30 bits.

I suggest you to use proper variable types, what about the following
snippet?

ktime_t launchtime;

launchtime = ktime_sub_ns(txtime, baset_est);
WARN_ON(upper_32_bits(launchtime));
div_s64_rem(launchtime, cycle_time, &launchtime);

return cpu_to_le32(lower_32_bits(launchtime));

> 
> However, I do not see on first inspection where that case is properly handled
> (probably just by rejecting the TAPRIO schedule).
> 
> Can someone with more experience in that area please jump in?
> 
> Thanks,
> Florian
> 
> > 
> >>  	s64 n;
> >>  
> >>  	n = div64_s64(ktime_sub_ns(now, base_time), cycle_time);
> >> -- 
> >> 2.38.1
> >>
> >>
> 

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

[Jakub Kicinski]

On Tue, 11 Jul 2023 13:12:33 +0300 Leon Romanovsky wrote:
> > If I understand correctly, ktime_sub_ns(txtime, baset_est) will only return
> > something larger than s32 max if cycle_time is larger than s32 max and if that
> > is the case everything will be broken anyway since the corresponding hardware
> > register only holds 30 bits.  
> 
> I suggest you to use proper variable types, what about the following
> snippet?
> 
> ktime_t launchtime;
> 
> launchtime = ktime_sub_ns(txtime, baset_est);
> WARN_ON(upper_32_bits(launchtime));
> div_s64_rem(launchtime, cycle_time, &launchtime);
> 
> return cpu_to_le32(lower_32_bits(launchtime));

That needs to be coupled with some control path checks on cycle_time?
Seems like a separate fix TBH.

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

[Leon Romanovsky]

On Tue, Jul 11, 2023 at 05:54:54PM -0700, Jakub Kicinski wrote:
> On Tue, 11 Jul 2023 13:12:33 +0300 Leon Romanovsky wrote:
> > > If I understand correctly, ktime_sub_ns(txtime, baset_est) will only return
> > > something larger than s32 max if cycle_time is larger than s32 max and if that
> > > is the case everything will be broken anyway since the corresponding hardware
> > > register only holds 30 bits.  
> > 
> > I suggest you to use proper variable types, what about the following
> > snippet?
> > 
> > ktime_t launchtime;
> > 
> > launchtime = ktime_sub_ns(txtime, baset_est);
> > WARN_ON(upper_32_bits(launchtime));
> > div_s64_rem(launchtime, cycle_time, &launchtime);
> > 
> > return cpu_to_le32(lower_32_bits(launchtime));
> 
> That needs to be coupled with some control path checks on cycle_time?

I think so, as I didn't find any checks which protect from overflow.

Thanks

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

[Tony Nguyen]

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

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>
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 | 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 4855caa3bae4..9f93f0f4f752 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1029,7 +1029,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.38.1

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

[Leon Romanovsky]

On Mon, Jul 10, 2023 at 09:35:03AM -0700, Tony Nguyen wrote:
> From: Florian Kauer <florian.kauer@linutronix.de>
> 
> 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>
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Tony Nguyen <anthony.l.nguyen@intel.com>:

On Mon, 10 Jul 2023 09:34:57 -0700 you wrote:
> Florian Kauer says:
> 
> 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.
> 
> [...]

Here is the summary with links:
  - [net,1/6] igc: Rename qbv_enable to taprio_offload_enable
    https://git.kernel.org/netdev/net/c/8046063df887
  - [net,2/6] igc: Do not enable taprio offload for invalid arguments
    https://git.kernel.org/netdev/net/c/82ff5f29b737
  - [net,3/6] igc: Handle already enabled taprio offload for basetime 0
    https://git.kernel.org/netdev/net/c/e5d88c53d03f
  - [net,4/6] igc: No strict mode in pure launchtime/CBS offload
    https://git.kernel.org/netdev/net/c/8b86f10ab64e
  - [net,5/6] igc: Fix launchtime before start of cycle
    https://git.kernel.org/netdev/net/c/c1bca9ac0bcb
  - [net,6/6] igc: Fix inserting of empty frame for launchtime
    https://git.kernel.org/netdev/net/c/0bcc62858d6b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html