[PATCH iwl-next] igc: Restore default Qbv schedule when changing channels

[PATCH iwl-next] igc: Restore default Qbv schedule when changing channels

[Kurt Kanzenbach]

The MQPRIO (and ETF) offload utilizes the TSN Tx mode. This mode is always
coupled to Qbv. Therefore, the driver sets a default Qbv schedule of all gates
opened and a cycle time of 1s. This schedule is set during probe.

However, the following sequence of events lead to Tx issues:

 - Boot a dual core system
   probe():
     igc_tsn_clear_schedule():
       -> Default Schedule is set
       Note: At this point the driver has allocated two Tx/Rx queues, because
       there are only two CPU(s).

 - ethtool -L enp3s0 combined 4
   igc_ethtool_set_channels():
     igc_reinit_queues()
       -> Default schedule is gone, per Tx ring start and end time are zero

  - tc qdisc replace dev enp3s0 handle 100 parent root mqprio \
      num_tc 4 map 3 3 2 2 0 1 1 1 3 3 3 3 3 3 3 3 \
      queues 1@0 1@1 1@2 1@3 hw 1
    igc_tsn_offload_apply():
      igc_tsn_enable_offload():
        -> Writes zeros to IGC_STQT(i) and IGC_ENDQT(i) -> Boom

Therefore, restore the default Qbv schedule after changing the amount of
channels.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 728d7ca5338bf27c3ce50a2a497b238c38cfa338..e4200fcb2682ccd5b57abb0d2b8e4eb30df117df 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -7761,6 +7761,8 @@ int igc_reinit_queues(struct igc_adapter *adapter)
 	if (netif_running(netdev))
 		err = igc_open(netdev);
 
+	igc_tsn_clear_schedule(adapter);
+
 	return err;
 }
 

---
base-commit: 6fc33710cd6c55397e606eeb544bdf56ee87aae5
change-id: 20251107-igc_mqprio_channels-2329166e898b

Best regards,
-- 
Kurt Kanzenbach <kurt@linutronix.de>

RE: [Intel-wired-lan] [PATCH iwl-next] igc: Restore default Qbv schedule when changing channels

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kurt Kanzenbach
> Sent: Friday, November 7, 2025 2:32 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Sebastian
> Andrzej Siewior <bigeasy@linutronix.de>; Gomes, Vinicius
> <vinicius.gomes@intel.com>; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; Kurt Kanzenbach <kurt@linutronix.de>
> Subject: [Intel-wired-lan] [PATCH iwl-next] igc: Restore default Qbv
> schedule when changing channels
> 
> The MQPRIO (and ETF) offload utilizes the TSN Tx mode. This mode is
> always coupled to Qbv. Therefore, the driver sets a default Qbv
> schedule of all gates opened and a cycle time of 1s. This schedule is
> set during probe.
> 
I'd recommend to explain abbreviations in the commit message:
  “Multi‑Queue Priority (MQPRIO)”
  “Earliest TxTime First (ETF)”
  “Time‑Sensitive Networking (TSN)”
  “Qbv” → “IEEE 802.1Qbv time‑aware shaper (Qbv)”

> However, the following sequence of events lead to Tx issues:
> 
>  - Boot a dual core system
>    probe():
>      igc_tsn_clear_schedule():
>        -> Default Schedule is set
>        Note: At this point the driver has allocated two Tx/Rx queues,
> because
>        there are only two CPU(s).
> 
>  - ethtool -L enp3s0 combined 4
>    igc_ethtool_set_channels():
>      igc_reinit_queues()
>        -> Default schedule is gone, per Tx ring start and end time are
> zero
> 
>   - tc qdisc replace dev enp3s0 handle 100 parent root mqprio \
>       num_tc 4 map 3 3 2 2 0 1 1 1 3 3 3 3 3 3 3 3 \
>       queues 1@0 1@1 1@2 1@3 hw 1
>     igc_tsn_offload_apply():
>       igc_tsn_enable_offload():
>         -> Writes zeros to IGC_STQT(i) and IGC_ENDQT(i) -> Boom
> 
> Therefore, restore the default Qbv schedule after changing the amount
> of channels.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
> b/drivers/net/ethernet/intel/igc/igc_main.c
> index
> 728d7ca5338bf27c3ce50a2a497b238c38cfa338..e4200fcb2682ccd5b57abb0d2b8e
> 4eb30df117df 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -7761,6 +7761,8 @@ int igc_reinit_queues(struct igc_adapter
> *adapter)
>  	if (netif_running(netdev))
>  		err = igc_open(netdev);
> 
> +	igc_tsn_clear_schedule(adapter);
> +
I'm afraid you need to guard the helper call on success (or when open wasn’t attempted)
Because call made even when igc_open() fails.
As written, igc_tsn_clear_schedule(adapter); executes unconditionally, even if igc_open()
returned an error (e.g., rings not fully set up, device not ready).
That could program TSN/Qbv registers while the device is in a failed/partially initialized state.
Isn't it?

       if (!netif_running(netdev) || !err) {
               /* Restore default IEEE 802.1Qbv schedule after queue reinit */
               igc_tsn_clear_schedule(adapter);
       }


>  	return err;
>  }
> 
> 
> ---
> base-commit: 6fc33710cd6c55397e606eeb544bdf56ee87aae5
> change-id: 20251107-igc_mqprio_channels-2329166e898b
> 
> Best regards,
> --
> Kurt Kanzenbach <kurt@linutronix.de>

RE: [Intel-wired-lan] [PATCH iwl-next] igc: Restore default Qbv schedule when changing channels

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 3526 bytes --]

On Fri Nov 07 2025, Loktionov, Aleksandr wrote:
>> -----Original Message-----
>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
>> Of Kurt Kanzenbach
>> Sent: Friday, November 7, 2025 2:32 PM
>> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
>> Przemyslaw <przemyslaw.kitszel@intel.com>
>> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
>> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
>> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Sebastian
>> Andrzej Siewior <bigeasy@linutronix.de>; Gomes, Vinicius
>> <vinicius.gomes@intel.com>; intel-wired-lan@lists.osuosl.org;
>> netdev@vger.kernel.org; Kurt Kanzenbach <kurt@linutronix.de>
>> Subject: [Intel-wired-lan] [PATCH iwl-next] igc: Restore default Qbv
>> schedule when changing channels
>> 
>> The MQPRIO (and ETF) offload utilizes the TSN Tx mode. This mode is
>> always coupled to Qbv. Therefore, the driver sets a default Qbv
>> schedule of all gates opened and a cycle time of 1s. This schedule is
>> set during probe.
>> 
> I'd recommend to explain abbreviations in the commit message:
>   “Multi‑Queue Priority (MQPRIO)”
>   “Earliest TxTime First (ETF)”
>   “Time‑Sensitive Networking (TSN)”
>   “Qbv” → “IEEE 802.1Qbv time‑aware shaper (Qbv)”
>
>> However, the following sequence of events lead to Tx issues:
>> 
>>  - Boot a dual core system
>>    probe():
>>      igc_tsn_clear_schedule():
>>        -> Default Schedule is set
>>        Note: At this point the driver has allocated two Tx/Rx queues,
>> because
>>        there are only two CPU(s).
>> 
>>  - ethtool -L enp3s0 combined 4
>>    igc_ethtool_set_channels():
>>      igc_reinit_queues()
>>        -> Default schedule is gone, per Tx ring start and end time are
>> zero
>> 
>>   - tc qdisc replace dev enp3s0 handle 100 parent root mqprio \
>>       num_tc 4 map 3 3 2 2 0 1 1 1 3 3 3 3 3 3 3 3 \
>>       queues 1@0 1@1 1@2 1@3 hw 1
>>     igc_tsn_offload_apply():
>>       igc_tsn_enable_offload():
>>         -> Writes zeros to IGC_STQT(i) and IGC_ENDQT(i) -> Boom
>> 
>> Therefore, restore the default Qbv schedule after changing the amount
>> of channels.
>> 
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
>>  drivers/net/ethernet/intel/igc/igc_main.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
>> b/drivers/net/ethernet/intel/igc/igc_main.c
>> index
>> 728d7ca5338bf27c3ce50a2a497b238c38cfa338..e4200fcb2682ccd5b57abb0d2b8e
>> 4eb30df117df 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -7761,6 +7761,8 @@ int igc_reinit_queues(struct igc_adapter
>> *adapter)
>>  	if (netif_running(netdev))
>>  		err = igc_open(netdev);
>> 
>> +	igc_tsn_clear_schedule(adapter);
>> +
> I'm afraid you need to guard the helper call on success (or when open wasn’t attempted)
> Because call made even when igc_open() fails.
> As written, igc_tsn_clear_schedule(adapter); executes unconditionally, even if igc_open()
> returned an error (e.g., rings not fully set up, device not ready).
> That could program TSN/Qbv registers while the device is in a failed/partially initialized state.
> Isn't it?

igc_tsn_clear_schedule() does not write any registers. It just sets the
default parameters. The actual programming is done later by
igc_tsn_enable_offload().

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH iwl-next] igc: Restore default Qbv schedule when changing channels

[Vinicius Costa Gomes]

Hi,

Kurt Kanzenbach <kurt@linutronix.de> writes:

> The MQPRIO (and ETF) offload utilizes the TSN Tx mode. This mode is always
> coupled to Qbv. Therefore, the driver sets a default Qbv schedule of all gates
> opened and a cycle time of 1s. This schedule is set during probe.
>
> However, the following sequence of events lead to Tx issues:
>
>  - Boot a dual core system
>    probe():
>      igc_tsn_clear_schedule():
>        -> Default Schedule is set
>        Note: At this point the driver has allocated two Tx/Rx queues, because
>        there are only two CPU(s).
>
>  - ethtool -L enp3s0 combined 4
>    igc_ethtool_set_channels():
>      igc_reinit_queues()
>        -> Default schedule is gone, per Tx ring start and end time are zero
>
>   - tc qdisc replace dev enp3s0 handle 100 parent root mqprio \
>       num_tc 4 map 3 3 2 2 0 1 1 1 3 3 3 3 3 3 3 3 \
>       queues 1@0 1@1 1@2 1@3 hw 1
>     igc_tsn_offload_apply():
>       igc_tsn_enable_offload():
>         -> Writes zeros to IGC_STQT(i) and IGC_ENDQT(i) -> Boom
>
> Therefore, restore the default Qbv schedule after changing the amount of
> channels.
>

Couple of questions:
 - Would it make sense to mark this patch as a fix?

 - What would happen if the user added a Qbv schedule (not the default
   one) and then changed the number of queues? My concern is that 'tc
   qdisc' would show the custom user schedule and the hardware would be
   "running" the default schedule, this inconsistency is not ideal. In
   any case, it would be a separate patch.


> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 728d7ca5338bf27c3ce50a2a497b238c38cfa338..e4200fcb2682ccd5b57abb0d2b8e4eb30df117df 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -7761,6 +7761,8 @@ int igc_reinit_queues(struct igc_adapter *adapter)
>  	if (netif_running(netdev))
>  		err = igc_open(netdev);
>  
> +	igc_tsn_clear_schedule(adapter);
> +
>  	return err;
>  }
>  
>
> ---
> base-commit: 6fc33710cd6c55397e606eeb544bdf56ee87aae5
> change-id: 20251107-igc_mqprio_channels-2329166e898b
>
> Best regards,
> -- 
> Kurt Kanzenbach <kurt@linutronix.de>
>


Cheers,
-- 
Vinicius

Re: [PATCH iwl-next] igc: Restore default Qbv schedule when changing channels

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

On Wed Nov 12 2025, Vinicius Costa Gomes wrote:
> Hi,
>
> Kurt Kanzenbach <kurt@linutronix.de> writes:
>
>> The MQPRIO (and ETF) offload utilizes the TSN Tx mode. This mode is always
>> coupled to Qbv. Therefore, the driver sets a default Qbv schedule of all gates
>> opened and a cycle time of 1s. This schedule is set during probe.
>>
>> However, the following sequence of events lead to Tx issues:
>>
>>  - Boot a dual core system
>>    probe():
>>      igc_tsn_clear_schedule():
>>        -> Default Schedule is set
>>        Note: At this point the driver has allocated two Tx/Rx queues, because
>>        there are only two CPU(s).
>>
>>  - ethtool -L enp3s0 combined 4
>>    igc_ethtool_set_channels():
>>      igc_reinit_queues()
>>        -> Default schedule is gone, per Tx ring start and end time are zero
>>
>>   - tc qdisc replace dev enp3s0 handle 100 parent root mqprio \
>>       num_tc 4 map 3 3 2 2 0 1 1 1 3 3 3 3 3 3 3 3 \
>>       queues 1@0 1@1 1@2 1@3 hw 1
>>     igc_tsn_offload_apply():
>>       igc_tsn_enable_offload():
>>         -> Writes zeros to IGC_STQT(i) and IGC_ENDQT(i) -> Boom
>>
>> Therefore, restore the default Qbv schedule after changing the amount of
>> channels.
>>
>
> Couple of questions:
>  - Would it make sense to mark this patch as a fix?

This only happens if a user uses ETF or MQPRIO and a dual/single core
system. So I didn't see the need to mark it as a fix.

>
>  - What would happen if the user added a Qbv schedule (not the default
>    one) and then changed the number of queues? My concern is that 'tc
>    qdisc' would show the custom user schedule and the hardware would be
>    "running" the default schedule, this inconsistency is not ideal. In
>    any case, it would be a separate patch.

Excellent point. Honestly I'm not sure what to expect when changing the
number of queues after a user Qbv schedule is added. For MQPRIO we added
a restriction [1] especially for that case. I'm leaning towards the same
solution here. What do you think?

Thanks,
Kurt

[1] - https://elixir.bootlin.com/linux/v6.18-rc5/source/drivers/net/ethernet/intel/igc/igc_ethtool.c#L1564

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH iwl-next] igc: Restore default Qbv schedule when changing channels

[Vinicius Costa Gomes]

Kurt Kanzenbach <kurt@linutronix.de> writes:

> On Wed Nov 12 2025, Vinicius Costa Gomes wrote:
>> Hi,
>>
>> Kurt Kanzenbach <kurt@linutronix.de> writes:
>>
>>> The MQPRIO (and ETF) offload utilizes the TSN Tx mode. This mode is always
>>> coupled to Qbv. Therefore, the driver sets a default Qbv schedule of all gates
>>> opened and a cycle time of 1s. This schedule is set during probe.
>>>
>>> However, the following sequence of events lead to Tx issues:
>>>
>>>  - Boot a dual core system
>>>    probe():
>>>      igc_tsn_clear_schedule():
>>>        -> Default Schedule is set
>>>        Note: At this point the driver has allocated two Tx/Rx queues, because
>>>        there are only two CPU(s).
>>>
>>>  - ethtool -L enp3s0 combined 4
>>>    igc_ethtool_set_channels():
>>>      igc_reinit_queues()
>>>        -> Default schedule is gone, per Tx ring start and end time are zero
>>>
>>>   - tc qdisc replace dev enp3s0 handle 100 parent root mqprio \
>>>       num_tc 4 map 3 3 2 2 0 1 1 1 3 3 3 3 3 3 3 3 \
>>>       queues 1@0 1@1 1@2 1@3 hw 1
>>>     igc_tsn_offload_apply():
>>>       igc_tsn_enable_offload():
>>>         -> Writes zeros to IGC_STQT(i) and IGC_ENDQT(i) -> Boom
>>>
>>> Therefore, restore the default Qbv schedule after changing the amount of
>>> channels.
>>>
>>
>> Couple of questions:
>>  - Would it make sense to mark this patch as a fix?
>
> This only happens if a user uses ETF or MQPRIO and a dual/single core
> system. So I didn't see the need to mark it as a fix.
>

I still think this is fix material. People can always run stuff in VMs,
and it makes it easier to have single/dual core systems.

>>
>>  - What would happen if the user added a Qbv schedule (not the default
>>    one) and then changed the number of queues? My concern is that 'tc
>>    qdisc' would show the custom user schedule and the hardware would be
>>    "running" the default schedule, this inconsistency is not ideal. In
>>    any case, it would be a separate patch.
>
> Excellent point. Honestly I'm not sure what to expect when changing the
> number of queues after a user Qbv schedule is added. For MQPRIO we added
> a restriction [1] especially for that case. I'm leaning towards the same
> solution here. What do you think?

Sounds great. Avoiding getting into inconsistent states is better than
trying to fix it later.

>
> Thanks,
> Kurt
>
> [1] - https://elixir.bootlin.com/linux/v6.18-rc5/source/drivers/net/ethernet/intel/igc/igc_ethtool.c#L1564


Cheers,
-- 
Vinicius

Re: [PATCH iwl-next] igc: Restore default Qbv schedule when changing channels

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]

On Thu Nov 13 2025, Vinicius Costa Gomes wrote:
> Kurt Kanzenbach <kurt@linutronix.de> writes:
>
>> On Wed Nov 12 2025, Vinicius Costa Gomes wrote:
>>> Hi,
>>>
>>> Kurt Kanzenbach <kurt@linutronix.de> writes:
>>>
>>>> The MQPRIO (and ETF) offload utilizes the TSN Tx mode. This mode is always
>>>> coupled to Qbv. Therefore, the driver sets a default Qbv schedule of all gates
>>>> opened and a cycle time of 1s. This schedule is set during probe.
>>>>
>>>> However, the following sequence of events lead to Tx issues:
>>>>
>>>>  - Boot a dual core system
>>>>    probe():
>>>>      igc_tsn_clear_schedule():
>>>>        -> Default Schedule is set
>>>>        Note: At this point the driver has allocated two Tx/Rx queues, because
>>>>        there are only two CPU(s).
>>>>
>>>>  - ethtool -L enp3s0 combined 4
>>>>    igc_ethtool_set_channels():
>>>>      igc_reinit_queues()
>>>>        -> Default schedule is gone, per Tx ring start and end time are zero
>>>>
>>>>   - tc qdisc replace dev enp3s0 handle 100 parent root mqprio \
>>>>       num_tc 4 map 3 3 2 2 0 1 1 1 3 3 3 3 3 3 3 3 \
>>>>       queues 1@0 1@1 1@2 1@3 hw 1
>>>>     igc_tsn_offload_apply():
>>>>       igc_tsn_enable_offload():
>>>>         -> Writes zeros to IGC_STQT(i) and IGC_ENDQT(i) -> Boom
>>>>
>>>> Therefore, restore the default Qbv schedule after changing the amount of
>>>> channels.
>>>>
>>>
>>> Couple of questions:
>>>  - Would it make sense to mark this patch as a fix?
>>
>> This only happens if a user uses ETF or MQPRIO and a dual/single core
>> system. So I didn't see the need to mark it as a fix.
>>
>
> I still think this is fix material. People can always run stuff in VMs,
> and it makes it easier to have single/dual core systems.

Fair enough.

>
>>>
>>>  - What would happen if the user added a Qbv schedule (not the default
>>>    one) and then changed the number of queues? My concern is that 'tc
>>>    qdisc' would show the custom user schedule and the hardware would be
>>>    "running" the default schedule, this inconsistency is not ideal. In
>>>    any case, it would be a separate patch.
>>
>> Excellent point. Honestly I'm not sure what to expect when changing the
>> number of queues after a user Qbv schedule is added. For MQPRIO we added
>> a restriction [1] especially for that case. I'm leaning towards the same
>> solution here. What do you think?
>
> Sounds great. Avoiding getting into inconsistent states is better than
> trying to fix it later.

Jup. I'll wait a bit for further comments and then send a v2 with your
and Aleksandr's suggestions addressed.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]