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

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- 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 --]