Hello Paul, On Mon Feb 26 2024, Paul Menzel wrote: > Dear Kurt, > > > Thank you for the patch. Some nits from my side. > > Am 26.02.24 um 09:51 schrieb Kurt Kanzenbach: >> Add support for offloading MQPRIO. The hardware has four priorities as well >> as four queues. Each queue must be a assigned with a unique priority. >> >> However, the priorities are only considered in TSN Tx mode. There are two >> TSN Tx modes. In case of MQPRIO the Qbv capability is not >> required. Therefore, use the legacy TSN Tx mode, which performs strict >> priority arbitration. > > You could reflow this paragraph. The second line breaks too early. Indeed, it does. > >> Example for mqprio with hardware offload: >> >> |tc qdisc replace dev ${INTERFACE} handle 100 parent root mqprio num_tc 4 \ >> | map 0 0 0 0 0 1 2 3 0 0 0 0 0 0 0 0 \ >> | queues 1@0 1@1 1@2 1@3 \ >> | hw 1 > > Thank you for the example. How can you check that it actually works? No > errors are shown? When using tc for setting up Qdisc(s) the command just returns without any messages. In case there's an error the user will see messages configured below with `extack`. > > Also, could you please mention the datasheet name, revision and section, > and on what exact device you tested this? Sure. > >> Signed-off-by: Kurt Kanzenbach >> --- >> drivers/net/ethernet/intel/igc/igc.h | 10 +++- >> drivers/net/ethernet/intel/igc/igc_defines.h | 9 ++++ >> drivers/net/ethernet/intel/igc/igc_main.c | 69 +++++++++++++++++++++++++++ >> drivers/net/ethernet/intel/igc/igc_regs.h | 2 + >> drivers/net/ethernet/intel/igc/igc_tsn.c | 71 +++++++++++++++++++++++++++- >> 5 files changed, 157 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h >> index 45430e246e9c..c95919f085eb 100644 >> --- a/drivers/net/ethernet/intel/igc/igc.h >> +++ b/drivers/net/ethernet/intel/igc/igc.h >> @@ -227,6 +227,10 @@ struct igc_adapter { >> */ >> spinlock_t qbv_tx_lock; >> >> + bool strict_priority_enable; >> + u8 num_tc; >> + u16 queue_per_tc[IGC_MAX_TX_QUEUES]; >> + >> /* OS defined structs */ >> struct pci_dev *pdev; >> /* lock for statistics */ >> @@ -343,9 +347,11 @@ extern char igc_driver_name[]; >> #define IGC_FLAG_RX_LEGACY BIT(16) >> #define IGC_FLAG_TSN_QBV_ENABLED BIT(17) >> #define IGC_FLAG_TSN_QAV_ENABLED BIT(18) >> +#define IGC_FLAG_TSN_LEGACY_ENABLED BIT(19) >> >> -#define IGC_FLAG_TSN_ANY_ENABLED \ >> - (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED) >> +#define IGC_FLAG_TSN_ANY_ENABLED \ >> + (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED | \ >> + IGC_FLAG_TSN_LEGACY_ENABLED) >> >> #define IGC_FLAG_RSS_FIELD_IPV4_UDP BIT(6) >> #define IGC_FLAG_RSS_FIELD_IPV6_UDP BIT(7) >> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h >> index 5f92b3c7c3d4..73502a0b4df7 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_defines.h >> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h >> @@ -547,6 +547,15 @@ >> >> #define IGC_MAX_SR_QUEUES 2 >> >> +#define IGC_TXARB_TXQ_PRIO_0_SHIFT 0 >> +#define IGC_TXARB_TXQ_PRIO_1_SHIFT 2 >> +#define IGC_TXARB_TXQ_PRIO_2_SHIFT 4 >> +#define IGC_TXARB_TXQ_PRIO_3_SHIFT 6 >> +#define IGC_TXARB_TXQ_PRIO_0_MASK GENMASK(1, 0) >> +#define IGC_TXARB_TXQ_PRIO_1_MASK GENMASK(3, 2) >> +#define IGC_TXARB_TXQ_PRIO_2_MASK GENMASK(5, 4) >> +#define IGC_TXARB_TXQ_PRIO_3_MASK GENMASK(7, 6) >> + >> /* Receive Checksum Control */ >> #define IGC_RXCSUM_CRCOFL 0x00000800 /* CRC32 offload enable */ >> #define IGC_RXCSUM_PCSD 0x00002000 /* packet checksum disabled */ >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index ba8d3fe186ae..c5d3cc3c4fa9 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -6424,6 +6424,13 @@ static int igc_tc_query_caps(struct igc_adapter *adapter, >> struct igc_hw *hw = &adapter->hw; >> >> switch (base->type) { >> + case TC_SETUP_QDISC_MQPRIO: { >> + struct tc_mqprio_caps *caps = base->caps; >> + >> + caps->validate_queue_counts = true; >> + >> + return 0; >> + } >> case TC_SETUP_QDISC_TAPRIO: { >> struct tc_taprio_caps *caps = base->caps; >> >> @@ -6441,6 +6448,65 @@ static int igc_tc_query_caps(struct igc_adapter *adapter, >> } >> } >> >> +static void igc_save_mqprio_params(struct igc_adapter *adapter, u8 num_tc, >> + u16 *offset) >> +{ >> + int i; >> + >> + adapter->strict_priority_enable = true; >> + adapter->num_tc = num_tc; >> + >> + for (i = 0; i < num_tc; i++) >> + adapter->queue_per_tc[i] = offset[i]; >> +} >> + >> +static int igc_tsn_enable_mqprio(struct igc_adapter *adapter, >> + struct tc_mqprio_qopt_offload *mqprio) >> +{ >> + struct igc_hw *hw = &adapter->hw; >> + int i; >> + >> + if (hw->mac.type != igc_i225) >> + return -EOPNOTSUPP; >> + >> + if (!mqprio->qopt.num_tc) { >> + adapter->strict_priority_enable = false; >> + goto apply; >> + } >> + >> + /* There are as many TCs as Tx queues. */ >> + if (mqprio->qopt.num_tc != adapter->num_tx_queues) { >> + NL_SET_ERR_MSG_FMT_MOD(mqprio->extack, >> + "Only %d traffic classes supported", >> + adapter->num_tx_queues); > > I’d also log `mqprio->qopt.num_tc`. num_tc is provided by the user. > >> + return -EOPNOTSUPP; >> + } >> + >> + /* Only one queue per TC is supported. */ >> + for (i = 0; i < mqprio->qopt.num_tc; i++) { >> + if (mqprio->qopt.count[i] != 1) { >> + NL_SET_ERR_MSG_MOD(mqprio->extack, >> + "Only one queue per TC supported"); > > I’d also log the i and the count value. Oki. > >> + return -EOPNOTSUPP; >> + } >> + } >> + >> + /* Preemption is not supported yet. */ >> + if (mqprio->preemptible_tcs) { >> + NL_SET_ERR_MSG_MOD(mqprio->extack, >> + "Preemption is not supported yet"); >> + return -EOPNOTSUPP; >> + } > > Please mention this in the commit message, maybe also included, if a > patch is coming soon, or what would need to be done to implement it. Oki. At some point i'd like to see FPE implemented for igc :-). Thanks, Kurt