RE: [PATCH iwl-net v2] igc: Restore default Qbv schedule when changing channels

[Kurt Kanzenbach <kurt@linutronix.de>]

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

On Tue Nov 18 2025, Loktionov, Aleksandr wrote:
>> -----Original Message-----
>> From: Kurt Kanzenbach <kurt@linutronix.de>
>> Sent: Tuesday, November 18, 2025 8:29 AM
>> 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>; Loktionov, Aleksandr
>> <aleksandr.loktionov@intel.com>; intel-wired-lan@lists.osuosl.org;
>> netdev@vger.kernel.org; Kurt Kanzenbach <kurt@linutronix.de>
>> Subject: [PATCH iwl-net v2] igc: Restore default Qbv schedule when
>> changing channels
>> 
>> The Multi Queue Priority (MQPRIO) and Earliest TxTime First (ETF)
>> offload utilizes the Time Sensitive Networking (TSN) Tx mode. This
> With two items (“MQPRIO and ETF”), the noun phrase is plural; verb must be utilize.
> BTW kernel docs and code commonly use “Multi‑queue”  with hyphen: "The Multi-queue Priority (MQPRIO) ..."
>
> s/Multi Queue/Multi-queue/
> s/offload/offloads/
> s/utilizes/utilize/
>
>> mode is always coupled to IEEE 802.1Qbv time aware shaper (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
>>    igc_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).
> Use standard plural: 'CPUs'
>
>> 
>>  - 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
> Please avoid colloquialism in commit logs. Suggest:
> "... IGC_STQT(i) and IGC_ENDQT(i), causing TX to stall/fail."
>
>
>> 
>> Therefore, restore the default Qbv schedule after changing the number
>> of channels.
>> 
>> Furthermore, add a restriction to not allow queue reconfiguration when
>> TSN/Qbv is enabled, because it may lead to inconsistent states.
>> 
>> Fixes: c814a2d2d48f ("igc: Use default cycle 'start' and 'end' values
>> for queues")
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
>> Changes in v2:
>> - Explain abbreviations (Aleksandr)
>> - Only clear schedule if no error happened (Aleksandr)
>> - Add restriction to avoid inconsistent states (Vinicius)
>> - Target net tree (Vinicius)
>> - Link to v1: https://lore.kernel.org/r/20251107-igc_mqprio_channels-
>> v1-1-42415562d0f8@linutronix.de
>> ---
>>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 4 ++--
>>  drivers/net/ethernet/intel/igc/igc_main.c    | 5 +++++
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> index
>> bb783042d1af9c86f18fc033fa4c3e75abb0efe8..43aea9e53e1e79b304c2a7e41fa7
>> d52dc956bffc 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> @@ -1561,8 +1561,8 @@ static int igc_ethtool_set_channels(struct
>> net_device *netdev,
>>  	if (ch->other_count != NON_Q_VECTORS)
>>  		return -EINVAL;
>> 
>> -	/* Do not allow channel reconfiguration when mqprio is enabled
>> */
>> -	if (adapter->strict_priority_enable)
>> +	/* Do not allow channel reconfiguration when any TSN Qdisc is
>> enabled */
> Kernel consistently spells the queuing discipline as “qdisc” (lowercase) in comments, Kconfig, and docs.

Sure, will update the changelog and comment in next version.

Thanks,
Kurt

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