Message ID | 20230602012827.25938-1-muhammad.husaini.zulkifli@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [iwl-net,v2] igc: Fix TX Hang issue when QBV Gate is close | expand |
Dear Muhammad, Thank you for your patch. Am 02.06.23 um 03:28 schrieb Muhammad Husaini Zulkifli: There is a small typo in the commit message summary/title: close*d*. > If a user schedules a Gate Control List (GCL) to close one of > the QBV gates while also transmitting a packet to that closed gate, > TX Hang will be happen. HW would not drop any packet when the gate > is close and keep queueing up in HW TX FIFO until the gate is re-open. 1. close*d* 2. queuing 3. re-opened > This patch implement the solution to drop the packet for the closed > gate. implement*s* > This patch will additionally include a reset adapter to perform Maybe: Additionally reset the adapter to β¦ > SW initialization for each 1st Gate Control List (GCL) to avoid hang. > This is due to the HW design, where changing to TSN transmit mode > requires SW initialization. Intel Discrete I225/6 transmit mode > cannot be changed when in dynamic mode according to Software User > Manual Section 7.5.2.1. Subsequent Gate Control List (GCL) operations > will proceed without a reset, as they already in TSN Mode. β¦ they already *are* β¦ > Step to reproduce: > > DUT: > 1) Configure GCL List with certain gate close. > 2) Transmit the packet to close gate. close*d* Do you have the commands to do what you did? > Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading") > Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com> > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> > Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com> > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > > --- > V1 -> V2: Fix conflict and apply to net-queue tree. > --- > --- > drivers/net/ethernet/intel/igc/igc.h | 6 +++ > drivers/net/ethernet/intel/igc/igc_main.c | 57 +++++++++++++++++++++-- > drivers/net/ethernet/intel/igc/igc_tsn.c | 41 ++++++++++------ > 3 files changed, 87 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index 0bbd108f28939..127b248054e55 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -13,6 +13,7 @@ > #include <linux/ptp_clock_kernel.h> > #include <linux/timecounter.h> > #include <linux/net_tstamp.h> > +#include <linux/hrtimer.h> > > #include "igc_hw.h" > > @@ -100,6 +101,8 @@ struct igc_ring { > u32 start_time; > u32 end_time; > u32 max_sdu; > + bool oper_gate_closed; What does *oper* refer to? > + bool admin_gate_closed; > > /* CBS parameters */ > bool cbs_enable; /* indicates if CBS is enabled */ > @@ -159,6 +162,7 @@ struct igc_adapter { > struct timer_list watchdog_timer; > struct timer_list dma_err_timer; > struct timer_list phy_info_timer; > + struct hrtimer hrtimer; > > u32 wol; > u32 en_mng_pt; > @@ -188,6 +192,8 @@ struct igc_adapter { > ktime_t cycle_time; > bool qbv_enable; > u32 qbv_config_change_errors; > + bool qbv_transition; > + int qbv_count; unsigned int? > /* OS defined structs */ > struct pci_dev *pdev; > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 9095306323afd..1f95376f9ffda 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -1521,6 +1521,9 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, > u8 hdr_len = 0; > int tso = 0; > > + if (adapter->qbv_transition || tx_ring->oper_gate_closed) > + goto out_drop; > + > /* need: 1 descriptor per page * PAGE_SIZE/IGC_MAX_DATA_PER_TXD, > * + 1 desc for skb_headlen/IGC_MAX_DATA_PER_TXD, > * + 2 desc gap to keep tail from touching head, > @@ -2967,7 +2970,8 @@ static bool igc_clean_tx_irq(struct igc_q_vector *q_vector, int napi_budget) > (adapter->tx_timeout_factor * HZ)) && > !(rd32(IGC_STATUS) & IGC_STATUS_TXOFF) && > (rd32(IGC_TDH(tx_ring->reg_idx)) != > - readl(tx_ring->tail))) { > + readl(tx_ring->tail)) && > + !tx_ring->oper_gate_closed) { > /* detected Tx unit hang */ > netdev_err(tx_ring->netdev, > "Detected Tx Unit Hang\n" > @@ -6057,6 +6061,8 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter) > adapter->base_time = 0; > adapter->cycle_time = NSEC_PER_SEC; > adapter->qbv_config_change_errors = 0; > + adapter->qbv_transition = false; > + adapter->qbv_count = 0; > > for (i = 0; i < adapter->num_tx_queues; i++) { > struct igc_ring *ring = adapter->tx_ring[i]; > @@ -6064,6 +6070,8 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter) > ring->start_time = 0; > ring->end_time = NSEC_PER_SEC; > ring->max_sdu = 0; > + ring->oper_gate_closed = false; > + ring->admin_gate_closed = false; > } > > return 0; > @@ -6075,6 +6083,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, > bool queue_configured[IGC_MAX_TX_QUEUES] = { }; > struct igc_hw *hw = &adapter->hw; > u32 start_time = 0, end_time = 0; > + struct timespec64 now; > size_t n; > int i; > > @@ -6095,6 +6104,8 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, > adapter->cycle_time = qopt->cycle_time; > adapter->base_time = qopt->base_time; > > + igc_ptp_read(adapter, &now); > + > for (n = 0; n < qopt->num_entries; n++) { > struct tc_taprio_sched_entry *e = &qopt->entries[n]; > > @@ -6129,7 +6140,10 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, > ring->start_time = start_time; > ring->end_time = end_time; > > - queue_configured[i] = true; > + if (ring->start_time >= adapter->cycle_time) > + queue_configured[i] = false; > + else > + queue_configured[i] = true; Maybe: queue_configured[i] = !(ring->start_time >= adapter->cycle_time) > } > > start_time += e->interval; > @@ -6139,8 +6153,20 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, > * If not, set the start and end time to be end time. > */ > for (i = 0; i < adapter->num_tx_queues; i++) { > + struct igc_ring *ring = adapter->tx_ring[i]; > + > + if (!is_base_time_past(qopt->base_time, &now)) { > + ring->admin_gate_closed = false; > + } else { > + ring->oper_gate_closed = false; > + ring->admin_gate_closed = false; > + } > + > if (!queue_configured[i]) { > - struct igc_ring *ring = adapter->tx_ring[i]; > + if (!is_base_time_past(qopt->base_time, &now)) > + ring->admin_gate_closed = true; > + else > + ring->oper_gate_closed = true; > > ring->start_time = end_time; > ring->end_time = end_time; > @@ -6466,6 +6492,27 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg) > return value; > } > > +static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer) > +{ > + struct igc_adapter *adapter = container_of(timer, struct igc_adapter, > + hrtimer); > + int i; unsigned int? > + > + adapter->qbv_transition = true; > + for (i = 0; i < adapter->num_tx_queues; i++) { > + struct igc_ring *tx_ring = adapter->tx_ring[i]; > + > + if (tx_ring->admin_gate_closed) { > + tx_ring->admin_gate_closed = false; > + tx_ring->oper_gate_closed = true; > + } else { > + tx_ring->oper_gate_closed = false; > + } > + } > + adapter->qbv_transition = false; > + return HRTIMER_NORESTART; > +} > + > /** > * igc_probe - Device Initialization Routine > * @pdev: PCI device information struct > @@ -6642,6 +6689,9 @@ static int igc_probe(struct pci_dev *pdev, > INIT_WORK(&adapter->reset_task, igc_reset_task); > INIT_WORK(&adapter->watchdog_task, igc_watchdog_task); > > + hrtimer_init(&adapter->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > + adapter->hrtimer.function = &igc_qbv_scheduling_timer; > + > /* Initialize link properties that are user-changeable */ > adapter->fc_autoneg = true; > hw->mac.autoneg = true; > @@ -6745,6 +6795,7 @@ static void igc_remove(struct pci_dev *pdev) > > cancel_work_sync(&adapter->reset_task); > cancel_work_sync(&adapter->watchdog_task); > + hrtimer_cancel(&adapter->hrtimer); > > /* Release control of h/w to f/w. If f/w is AMT enabled, this > * would have already happened in close and is redundant. > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c > index 6b299b83e7ef2..81770955029dc 100644 > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > @@ -114,7 +114,6 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter) > static int igc_tsn_enable_offload(struct igc_adapter *adapter) > { > struct igc_hw *hw = &adapter->hw; > - bool tsn_mode_reconfig = false; > u32 tqavctrl, baset_l, baset_h; > u32 sec, nsec, cycle; > ktime_t base_time, systim; > @@ -228,11 +227,10 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) > > tqavctrl = rd32(IGC_TQAVCTRL) & ~IGC_TQAVCTRL_FUTSCDDIS; > > - if (tqavctrl & IGC_TQAVCTRL_TRANSMIT_MODE_TSN) > - tsn_mode_reconfig = true; > - > tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV; > > + adapter->qbv_count++; > + > cycle = adapter->cycle_time; > base_time = adapter->base_time; > > @@ -250,17 +248,28 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) > */ > if ((rd32(IGC_BASET_H) || rd32(IGC_BASET_L)) && > (adapter->tc_setup_type == TC_SETUP_QDISC_TAPRIO) && > - tsn_mode_reconfig) > + (adapter->qbv_count > 1)) > adapter->qbv_config_change_errors++; > } else { > - /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit > - * has to be configured before the cycle time and base time. > - * Tx won't hang if there is a GCL is already running, > - * so in this case we don't need to set FutScdDis. > - */ > - if (igc_is_device_id_i226(hw) && > - !(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) > - tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; > + if (igc_is_device_id_i226(hw)) { > + ktime_t adjust_time, expires_time; > + > + /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit > + * has to be configured before the cycle time and base time. > + * Tx won't hang if there is a GCL is already running, Remove second *is*? > + * so in this case we don't need to set FutScdDis. > + */ > + if (!(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) > + tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; > + > + nsec = rd32(IGC_SYSTIML); > + sec = rd32(IGC_SYSTIMH); > + systim = ktime_set(sec, nsec); > + > + adjust_time = adapter->base_time; > + expires_time = ktime_sub_ns(adjust_time, systim); > + hrtimer_start(&adapter->hrtimer, expires_time, HRTIMER_MODE_REL); > + } > } > > wr32(IGC_TQAVCTRL, tqavctrl); > @@ -306,7 +315,11 @@ int igc_tsn_offload_apply(struct igc_adapter *adapter) > { > struct igc_hw *hw = &adapter->hw; > > - if (netif_running(adapter->netdev) && igc_is_device_id_i225(hw)) { > + /* Per I225/6 HW Design Section 7.5.2.1, transmit mode > + * cannot be change dynamically. Require reset the adapter. change*d* > + */ > + if (netif_running(adapter->netdev) && > + (igc_is_device_id_i225(hw) || !adapter->qbv_count)) { > schedule_work(&adapter->reset_task); > return 0; > } Kind regards, Paul
Dear Paul, Thanks for the review. > -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: Friday, 2 June, 2023 8:03 PM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Cc: intel-wired-lan@osuosl.org; Choong, Chwee Lin > <chwee.lin.choong@intel.com>; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; tee.min.tan@linux.intel.com > Subject: Re: [Intel-wired-lan] [PATCH iwl-net v2] igc: Fix TX Hang issue when > QBV Gate is close > > Dear Muhammad, > > > Thank you for your patch. > > Am 02.06.23 um 03:28 schrieb Muhammad Husaini Zulkifli: > > There is a small typo in the commit message summary/title: close*d*. Noted. Already fix this. > > > If a user schedules a Gate Control List (GCL) to close one of the QBV > > gates while also transmitting a packet to that closed gate, TX Hang > > will be happen. HW would not drop any packet when the gate is close > > and keep queueing up in HW TX FIFO until the gate is re-open. > > 1. close*d* > 2. queuing > 3. re-opened Will change accordingly π > > > This patch implement the solution to drop the packet for the closed > > gate. > > implement*s* Yup > > > This patch will additionally include a reset adapter to perform > > Maybe: Additionally reset the adapter to β¦ Sure. I will rephrase it. > > > SW initialization for each 1st Gate Control List (GCL) to avoid hang. > > This is due to the HW design, where changing to TSN transmit mode > > requires SW initialization. Intel Discrete I225/6 transmit mode cannot > > be changed when in dynamic mode according to Software User Manual > > Section 7.5.2.1. Subsequent Gate Control List (GCL) operations will > > proceed without a reset, as they already in TSN Mode. > > β¦ they already *are* β¦ Yup. Thanks! > > > Step to reproduce: > > > > DUT: > > 1) Configure GCL List with certain gate close. > > 2) Transmit the packet to close gate. > > close*d* Noted. > > Do you have the commands to do what you did? Sure. I will update the command in commit message in v3. > > > Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading") > > Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com> > > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> > > Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com> > > Signed-off-by: Muhammad Husaini Zulkifli > > <muhammad.husaini.zulkifli@intel.com> > > > > --- > > V1 -> V2: Fix conflict and apply to net-queue tree. > > --- > > --- > > drivers/net/ethernet/intel/igc/igc.h | 6 +++ > > drivers/net/ethernet/intel/igc/igc_main.c | 57 +++++++++++++++++++++-- > > drivers/net/ethernet/intel/igc/igc_tsn.c | 41 ++++++++++------ > > 3 files changed, 87 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc.h > > b/drivers/net/ethernet/intel/igc/igc.h > > index 0bbd108f28939..127b248054e55 100644 > > --- a/drivers/net/ethernet/intel/igc/igc.h > > +++ b/drivers/net/ethernet/intel/igc/igc.h > > @@ -13,6 +13,7 @@ > > #include <linux/ptp_clock_kernel.h> > > #include <linux/timecounter.h> > > #include <linux/net_tstamp.h> > > +#include <linux/hrtimer.h> > > > > #include "igc_hw.h" > > > > @@ -100,6 +101,8 @@ struct igc_ring { > > u32 start_time; > > u32 end_time; > > u32 max_sdu; > > + bool oper_gate_closed; > > What does *oper* refer to? "Oper" refers to the operating gate, or "current gate" in this case. This variable indicate that the current queue gate is closed. > > > + bool admin_gate_closed; > > > > /* CBS parameters */ > > bool cbs_enable; /* indicates if CBS is enabled */ > > @@ -159,6 +162,7 @@ struct igc_adapter { > > struct timer_list watchdog_timer; > > struct timer_list dma_err_timer; > > struct timer_list phy_info_timer; > > + struct hrtimer hrtimer; > > > > u32 wol; > > u32 en_mng_pt; > > @@ -188,6 +192,8 @@ struct igc_adapter { > > ktime_t cycle_time; > > bool qbv_enable; > > u32 qbv_config_change_errors; > > + bool qbv_transition; > > + int qbv_count; > > unsigned int? Will do. > > > /* OS defined structs */ > > struct pci_dev *pdev; > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c > > b/drivers/net/ethernet/intel/igc/igc_main.c > > index 9095306323afd..1f95376f9ffda 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_main.c > > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > > @@ -1521,6 +1521,9 @@ static netdev_tx_t igc_xmit_frame_ring(struct > sk_buff *skb, > > u8 hdr_len = 0; > > int tso = 0; > > > > + if (adapter->qbv_transition || tx_ring->oper_gate_closed) > > + goto out_drop; > > + > > /* need: 1 descriptor per page * > PAGE_SIZE/IGC_MAX_DATA_PER_TXD, > > * + 1 desc for skb_headlen/IGC_MAX_DATA_PER_TXD, > > * + 2 desc gap to keep tail from touching head, > > @@ -2967,7 +2970,8 @@ static bool igc_clean_tx_irq(struct igc_q_vector > *q_vector, int napi_budget) > > (adapter->tx_timeout_factor * HZ)) && > > !(rd32(IGC_STATUS) & IGC_STATUS_TXOFF) && > > (rd32(IGC_TDH(tx_ring->reg_idx)) != > > - readl(tx_ring->tail))) { > > + readl(tx_ring->tail)) && > > + !tx_ring->oper_gate_closed) { > > /* detected Tx unit hang */ > > netdev_err(tx_ring->netdev, > > "Detected Tx Unit Hang\n" > > @@ -6057,6 +6061,8 @@ static int igc_tsn_clear_schedule(struct > igc_adapter *adapter) > > adapter->base_time = 0; > > adapter->cycle_time = NSEC_PER_SEC; > > adapter->qbv_config_change_errors = 0; > > + adapter->qbv_transition = false; > > + adapter->qbv_count = 0; > > > > for (i = 0; i < adapter->num_tx_queues; i++) { > > struct igc_ring *ring = adapter->tx_ring[i]; @@ -6064,6 > +6070,8 @@ > > static int igc_tsn_clear_schedule(struct igc_adapter *adapter) > > ring->start_time = 0; > > ring->end_time = NSEC_PER_SEC; > > ring->max_sdu = 0; > > + ring->oper_gate_closed = false; > > + ring->admin_gate_closed = false; > > } > > > > return 0; > > @@ -6075,6 +6083,7 @@ static int igc_save_qbv_schedule(struct > igc_adapter *adapter, > > bool queue_configured[IGC_MAX_TX_QUEUES] = { }; > > struct igc_hw *hw = &adapter->hw; > > u32 start_time = 0, end_time = 0; > > + struct timespec64 now; > > size_t n; > > int i; > > > > @@ -6095,6 +6104,8 @@ static int igc_save_qbv_schedule(struct > igc_adapter *adapter, > > adapter->cycle_time = qopt->cycle_time; > > adapter->base_time = qopt->base_time; > > > > + igc_ptp_read(adapter, &now); > > + > > for (n = 0; n < qopt->num_entries; n++) { > > struct tc_taprio_sched_entry *e = &qopt->entries[n]; > > > > @@ -6129,7 +6140,10 @@ static int igc_save_qbv_schedule(struct > igc_adapter *adapter, > > ring->start_time = start_time; > > ring->end_time = end_time; > > > > - queue_configured[i] = true; > > + if (ring->start_time >= adapter->cycle_time) > > + queue_configured[i] = false; > > + else > > + queue_configured[i] = true; > > Maybe: > > queue_configured[i] = !(ring->start_time >= adapter->cycle_time) I will maintain the original code for this one . Thanks for the suggestion. > > > } > > > > start_time += e->interval; > > @@ -6139,8 +6153,20 @@ static int igc_save_qbv_schedule(struct > igc_adapter *adapter, > > * If not, set the start and end time to be end time. > > */ > > for (i = 0; i < adapter->num_tx_queues; i++) { > > + struct igc_ring *ring = adapter->tx_ring[i]; > > + > > + if (!is_base_time_past(qopt->base_time, &now)) { > > + ring->admin_gate_closed = false; > > + } else { > > + ring->oper_gate_closed = false; > > + ring->admin_gate_closed = false; > > + } > > + > > if (!queue_configured[i]) { > > - struct igc_ring *ring = adapter->tx_ring[i]; > > + if (!is_base_time_past(qopt->base_time, &now)) > > + ring->admin_gate_closed = true; > > + else > > + ring->oper_gate_closed = true; > > > > ring->start_time = end_time; > > ring->end_time = end_time; > > @@ -6466,6 +6492,27 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg) > > return value; > > } > > > > +static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer > > +*timer) { > > + struct igc_adapter *adapter = container_of(timer, struct igc_adapter, > > + hrtimer); > > + int i; > > unsigned int? Absolutely will change it π > > > + > > + adapter->qbv_transition = true; > > + for (i = 0; i < adapter->num_tx_queues; i++) { > > + struct igc_ring *tx_ring = adapter->tx_ring[i]; > > + > > + if (tx_ring->admin_gate_closed) { > > + tx_ring->admin_gate_closed = false; > > + tx_ring->oper_gate_closed = true; > > + } else { > > + tx_ring->oper_gate_closed = false; > > + } > > + } > > + adapter->qbv_transition = false; > > + return HRTIMER_NORESTART; > > +} > > + > > /** > > * igc_probe - Device Initialization Routine > > * @pdev: PCI device information struct @@ -6642,6 +6689,9 @@ static > > int igc_probe(struct pci_dev *pdev, > > INIT_WORK(&adapter->reset_task, igc_reset_task); > > INIT_WORK(&adapter->watchdog_task, igc_watchdog_task); > > > > + hrtimer_init(&adapter->hrtimer, CLOCK_MONOTONIC, > HRTIMER_MODE_REL); > > + adapter->hrtimer.function = &igc_qbv_scheduling_timer; > > + > > /* Initialize link properties that are user-changeable */ > > adapter->fc_autoneg = true; > > hw->mac.autoneg = true; > > @@ -6745,6 +6795,7 @@ static void igc_remove(struct pci_dev *pdev) > > > > cancel_work_sync(&adapter->reset_task); > > cancel_work_sync(&adapter->watchdog_task); > > + hrtimer_cancel(&adapter->hrtimer); > > > > /* Release control of h/w to f/w. If f/w is AMT enabled, this > > * would have already happened in close and is redundant. > > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c > > b/drivers/net/ethernet/intel/igc/igc_tsn.c > > index 6b299b83e7ef2..81770955029dc 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > > @@ -114,7 +114,6 @@ static int igc_tsn_disable_offload(struct igc_adapter > *adapter) > > static int igc_tsn_enable_offload(struct igc_adapter *adapter) > > { > > struct igc_hw *hw = &adapter->hw; > > - bool tsn_mode_reconfig = false; > > u32 tqavctrl, baset_l, baset_h; > > u32 sec, nsec, cycle; > > ktime_t base_time, systim; > > @@ -228,11 +227,10 @@ static int igc_tsn_enable_offload(struct > > igc_adapter *adapter) > > > > tqavctrl = rd32(IGC_TQAVCTRL) & ~IGC_TQAVCTRL_FUTSCDDIS; > > > > - if (tqavctrl & IGC_TQAVCTRL_TRANSMIT_MODE_TSN) > > - tsn_mode_reconfig = true; > > - > > tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | > > IGC_TQAVCTRL_ENHANCED_QAV; > > > > + adapter->qbv_count++; > > + > > cycle = adapter->cycle_time; > > base_time = adapter->base_time; > > > > @@ -250,17 +248,28 @@ static int igc_tsn_enable_offload(struct > igc_adapter *adapter) > > */ > > if ((rd32(IGC_BASET_H) || rd32(IGC_BASET_L)) && > > (adapter->tc_setup_type == TC_SETUP_QDISC_TAPRIO) && > > - tsn_mode_reconfig) > > + (adapter->qbv_count > 1)) > > adapter->qbv_config_change_errors++; > > } else { > > - /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit > > - * has to be configured before the cycle time and base time. > > - * Tx won't hang if there is a GCL is already running, > > - * so in this case we don't need to set FutScdDis. > > - */ > > - if (igc_is_device_id_i226(hw) && > > - !(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) > > - tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; > > + if (igc_is_device_id_i226(hw)) { > > + ktime_t adjust_time, expires_time; > > + > > + /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit > > + * has to be configured before the cycle time and base > time. > > + * Tx won't hang if there is a GCL is already running, > > Remove second *is*? Thanks for noticed it. Will rephase and remove it. > > > + * so in this case we don't need to set FutScdDis. > > + */ > > + if (!(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) > > + tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; > > + > > + nsec = rd32(IGC_SYSTIML); > > + sec = rd32(IGC_SYSTIMH); > > + systim = ktime_set(sec, nsec); > > + > > + adjust_time = adapter->base_time; > > + expires_time = ktime_sub_ns(adjust_time, systim); > > + hrtimer_start(&adapter->hrtimer, expires_time, > HRTIMER_MODE_REL); > > + } > > } > > > > wr32(IGC_TQAVCTRL, tqavctrl); > > @@ -306,7 +315,11 @@ int igc_tsn_offload_apply(struct igc_adapter > *adapter) > > { > > struct igc_hw *hw = &adapter->hw; > > > > - if (netif_running(adapter->netdev) && igc_is_device_id_i225(hw)) { > > + /* Per I225/6 HW Design Section 7.5.2.1, transmit mode > > + * cannot be change dynamically. Require reset the adapter. > > change*d* Good catch! Thanks, Husaini > > > + */ > > + if (netif_running(adapter->netdev) && > > + (igc_is_device_id_i225(hw) || !adapter->qbv_count)) { > > schedule_work(&adapter->reset_task); > > return 0; > > } > > > Kind regards, > > Paul
On 6/1/2023 6:28 PM, Muhammad Husaini Zulkifli wrote: > If a user schedules a Gate Control List (GCL) to close one of > the QBV gates while also transmitting a packet to that closed gate, > TX Hang will be happen. HW would not drop any packet when the gate > is close and keep queueing up in HW TX FIFO until the gate is re-open. > This patch implement the solution to drop the packet for the closed > gate. > > This patch will additionally include a reset adapter to perform > SW initialization for each 1st Gate Control List (GCL) to avoid hang. > This is due to the HW design, where changing to TSN transmit mode > requires SW initialization. Intel Discrete I225/6 transmit mode > cannot be changed when in dynamic mode according to Software User > Manual Section 7.5.2.1. Subsequent Gate Control List (GCL) operations > will proceed without a reset, as they already in TSN Mode. > > Step to reproduce: > > DUT: > 1) Configure GCL List with certain gate close. > 2) Transmit the packet to close gate. > > Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading") > Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com> > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> > Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com> > Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> > No newline here please. > --- ... > V1 -> V2: Fix conflict and apply to net-queue tree. > --- > --- no need for two '---' > @@ -1521,6 +1521,9 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, > u8 hdr_len = 0; > int tso = 0; > > + if (adapter->qbv_transition || tx_ring->oper_gate_closed) > + goto out_drop; > + clang reports issues with this: +../drivers/net/ethernet/intel/igc/igc_main.c:1524:6: warning: variable 'first' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] + if (adapter->qbv_transition || tx_ring->oper_gate_closed) + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +../drivers/net/ethernet/intel/igc/igc_main.c:1632:20: note: uninitialized use occurs here + dev_kfree_skb_any(first->skb); + ^~~~~ +../drivers/net/ethernet/intel/igc/igc_main.c:1524:2: note: remove the 'if' if its condition is always false + if (adapter->qbv_transition || tx_ring->oper_gate_closed) + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +../drivers/net/ethernet/intel/igc/igc_main.c:1524:6: warning: variable 'first' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized] + if (adapter->qbv_transition || tx_ring->oper_gate_closed) + ^~~~~~~~~~~~~~~~~~~~~~~ +../drivers/net/ethernet/intel/igc/igc_main.c:1632:20: note: uninitialized use occurs here + dev_kfree_skb_any(first->skb); + ^~~~~ +../drivers/net/ethernet/intel/igc/igc_main.c:1524:6: note: remove the '||' if its condition is always false + if (adapter->qbv_transition || tx_ring->oper_gate_closed) + ^~~~~~~~~~~~~~~~~~~~~~~~~~ +../drivers/net/ethernet/intel/igc/igc_main.c:1516:29: note: initialize the variable 'first' to silence this warning + struct igc_tx_buffer *first; + ^ + = NULL +2 warnings generated. > /* need: 1 descriptor per page * PAGE_SIZE/IGC_MAX_DATA_PER_TXD, > * + 1 desc for skb_headlen/IGC_MAX_DATA_PER_TXD, > * + 2 desc gap to keep tail from touching head,
Dear Muhammad, Am 02.06.23 um 15:26 schrieb Zulkifli, Muhammad Husaini: >> -----Original Message----- >> From: Paul Menzel <pmenzel@molgen.mpg.de> >> Sent: Friday, 2 June, 2023 8:03 PM >> Am 02.06.23 um 03:28 schrieb Muhammad Husaini Zulkifli: >> There is a small typo in the commit message summary/title: close*d*. > > Noted. Already fix this. > >>> If a user schedules a Gate Control List (GCL) to close one of the QBV >>> gates while also transmitting a packet to that closed gate, TX Hang >>> will be happen. HW would not drop any packet when the gate is close >>> and keep queueing up in HW TX FIFO until the gate is re-open. >> >> 1. close*d* >> 2. queuing >> 3. re-opened > > Will change accordingly π > >> >>> This patch implement the solution to drop the packet for the closed >>> gate. >> >> implement*s* > > Yup > >>> This patch will additionally include a reset adapter to perform >> >> Maybe: Additionally reset the adapter to β¦ > > Sure. I will rephrase it. > >>> SW initialization for each 1st Gate Control List (GCL) to avoid hang. >>> This is due to the HW design, where changing to TSN transmit mode >>> requires SW initialization. Intel Discrete I225/6 transmit mode cannot >>> be changed when in dynamic mode according to Software User Manual >>> Section 7.5.2.1. Subsequent Gate Control List (GCL) operations will >>> proceed without a reset, as they already in TSN Mode. >> >> β¦ they already *are* β¦ > > Yup. Thanks! > >> >>> Step to reproduce: >>> >>> DUT: >>> 1) Configure GCL List with certain gate close. >>> 2) Transmit the packet to close gate. >> >> close*d* > > Noted. > >> >> Do you have the commands to do what you did? > > Sure. I will update the command in commit message in v3. > >> >>> Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading") >>> Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com> >>> Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> >>> Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com> >>> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com> >>> >>> --- >>> V1 -> V2: Fix conflict and apply to net-queue tree. >>> --- >>> --- >>> drivers/net/ethernet/intel/igc/igc.h | 6 +++ >>> drivers/net/ethernet/intel/igc/igc_main.c | 57 +++++++++++++++++++++-- >>> drivers/net/ethernet/intel/igc/igc_tsn.c | 41 ++++++++++------ >>> 3 files changed, 87 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/igc/igc.h >>> b/drivers/net/ethernet/intel/igc/igc.h >>> index 0bbd108f28939..127b248054e55 100644 >>> --- a/drivers/net/ethernet/intel/igc/igc.h >>> +++ b/drivers/net/ethernet/intel/igc/igc.h >>> @@ -13,6 +13,7 @@ >>> #include <linux/ptp_clock_kernel.h> >>> #include <linux/timecounter.h> >>> #include <linux/net_tstamp.h> >>> +#include <linux/hrtimer.h> >>> >>> #include "igc_hw.h" >>> >>> @@ -100,6 +101,8 @@ struct igc_ring { >>> u32 start_time; >>> u32 end_time; >>> u32 max_sdu; >>> + bool oper_gate_closed; >> >> What does *oper* refer to? > > "Oper" refers to the operating gate, or "current gate" in this case. > This variable indicate that the current queue gate is closed. Maybe add a comment on the same line: bool oper_gate_closed; /* Is operating/current queue gate closed? */ But maybe itβs common knowledge and can also be ommited. Your decision. Kind regards, Paul >>> + bool admin_gate_closed; >>> >>> /* CBS parameters */ >>> bool cbs_enable; /* indicates if CBS is enabled */ >>> @@ -159,6 +162,7 @@ struct igc_adapter { >>> struct timer_list watchdog_timer; >>> struct timer_list dma_err_timer; >>> struct timer_list phy_info_timer; >>> + struct hrtimer hrtimer; >>> >>> u32 wol; >>> u32 en_mng_pt; >>> @@ -188,6 +192,8 @@ struct igc_adapter { >>> ktime_t cycle_time; >>> bool qbv_enable; >>> u32 qbv_config_change_errors; >>> + bool qbv_transition; >>> + int qbv_count; >> >> unsigned int? > > Will do. > >> >>> /* OS defined structs */ >>> struct pci_dev *pdev; >>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c >>> b/drivers/net/ethernet/intel/igc/igc_main.c >>> index 9095306323afd..1f95376f9ffda 100644 >>> --- a/drivers/net/ethernet/intel/igc/igc_main.c >>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >>> @@ -1521,6 +1521,9 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, >>> u8 hdr_len = 0; >>> int tso = 0; >>> >>> + if (adapter->qbv_transition || tx_ring->oper_gate_closed) >>> + goto out_drop; >>> + >>> /* need: 1 descriptor per page * PAGE_SIZE/IGC_MAX_DATA_PER_TXD, >>> * + 1 desc for skb_headlen/IGC_MAX_DATA_PER_TXD, >>> * + 2 desc gap to keep tail from touching head, >>> @@ -2967,7 +2970,8 @@ static bool igc_clean_tx_irq(struct igc_q_vector *q_vector, int napi_budget) >>> (adapter->tx_timeout_factor * HZ)) && >>> !(rd32(IGC_STATUS) & IGC_STATUS_TXOFF) && >>> (rd32(IGC_TDH(tx_ring->reg_idx)) != >>> - readl(tx_ring->tail))) { >>> + readl(tx_ring->tail)) && >>> + !tx_ring->oper_gate_closed) { >>> /* detected Tx unit hang */ >>> netdev_err(tx_ring->netdev, >>> "Detected Tx Unit Hang\n" >>> @@ -6057,6 +6061,8 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter) >>> adapter->base_time = 0; >>> adapter->cycle_time = NSEC_PER_SEC; >>> adapter->qbv_config_change_errors = 0; >>> + adapter->qbv_transition = false; >>> + adapter->qbv_count = 0; >>> >>> for (i = 0; i < adapter->num_tx_queues; i++) { >>> struct igc_ring *ring = adapter->tx_ring[i]; @@ -6064,6 >> +6070,8 @@ >>> static int igc_tsn_clear_schedule(struct igc_adapter *adapter) >>> ring->start_time = 0; >>> ring->end_time = NSEC_PER_SEC; >>> ring->max_sdu = 0; >>> + ring->oper_gate_closed = false; >>> + ring->admin_gate_closed = false; >>> } >>> >>> return 0; >>> @@ -6075,6 +6083,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, >>> bool queue_configured[IGC_MAX_TX_QUEUES] = { }; >>> struct igc_hw *hw = &adapter->hw; >>> u32 start_time = 0, end_time = 0; >>> + struct timespec64 now; >>> size_t n; >>> int i; >>> >>> @@ -6095,6 +6104,8 @@ static int igc_save_qbv_schedule(struct >> igc_adapter *adapter, >>> adapter->cycle_time = qopt->cycle_time; >>> adapter->base_time = qopt->base_time; >>> >>> + igc_ptp_read(adapter, &now); >>> + >>> for (n = 0; n < qopt->num_entries; n++) { >>> struct tc_taprio_sched_entry *e = &qopt->entries[n]; >>> >>> @@ -6129,7 +6140,10 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, >>> ring->start_time = start_time; >>> ring->end_time = end_time; >>> >>> - queue_configured[i] = true; >>> + if (ring->start_time >= adapter->cycle_time) >>> + queue_configured[i] = false; >>> + else >>> + queue_configured[i] = true; >> >> Maybe: >> >> queue_configured[i] = !(ring->start_time >= adapter->cycle_time) > > I will maintain the original code for this one . Thanks for the suggestion. > >>> } >>> >>> start_time += e->interval; >>> @@ -6139,8 +6153,20 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, >>> * If not, set the start and end time to be end time. >>> */ >>> for (i = 0; i < adapter->num_tx_queues; i++) { >>> + struct igc_ring *ring = adapter->tx_ring[i]; >>> + >>> + if (!is_base_time_past(qopt->base_time, &now)) { >>> + ring->admin_gate_closed = false; >>> + } else { >>> + ring->oper_gate_closed = false; >>> + ring->admin_gate_closed = false; >>> + } >>> + >>> if (!queue_configured[i]) { >>> - struct igc_ring *ring = adapter->tx_ring[i]; >>> + if (!is_base_time_past(qopt->base_time, &now)) >>> + ring->admin_gate_closed = true; >>> + else >>> + ring->oper_gate_closed = true; >>> >>> ring->start_time = end_time; >>> ring->end_time = end_time; >>> @@ -6466,6 +6492,27 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg) >>> return value; >>> } >>> >>> +static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer +*timer) { >>> + struct igc_adapter *adapter = container_of(timer, struct igc_adapter, >>> + hrtimer); >>> + int i; >> >> unsigned int? > > Absolutely will change it π > >>> + >>> + adapter->qbv_transition = true; >>> + for (i = 0; i < adapter->num_tx_queues; i++) { >>> + struct igc_ring *tx_ring = adapter->tx_ring[i]; >>> + >>> + if (tx_ring->admin_gate_closed) { >>> + tx_ring->admin_gate_closed = false; >>> + tx_ring->oper_gate_closed = true; >>> + } else { >>> + tx_ring->oper_gate_closed = false; >>> + } >>> + } >>> + adapter->qbv_transition = false; >>> + return HRTIMER_NORESTART; >>> +} >>> + >>> /** >>> * igc_probe - Device Initialization Routine >>> * @pdev: PCI device information struct @@ -6642,6 +6689,9 @@ static >>> int igc_probe(struct pci_dev *pdev, >>> INIT_WORK(&adapter->reset_task, igc_reset_task); >>> INIT_WORK(&adapter->watchdog_task, igc_watchdog_task); >>> >>> + hrtimer_init(&adapter->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); >>> + adapter->hrtimer.function = &igc_qbv_scheduling_timer; >>> + >>> /* Initialize link properties that are user-changeable */ >>> adapter->fc_autoneg = true; >>> hw->mac.autoneg = true; >>> @@ -6745,6 +6795,7 @@ static void igc_remove(struct pci_dev *pdev) >>> >>> cancel_work_sync(&adapter->reset_task); >>> cancel_work_sync(&adapter->watchdog_task); >>> + hrtimer_cancel(&adapter->hrtimer); >>> >>> /* Release control of h/w to f/w. If f/w is AMT enabled, this >>> * would have already happened in close and is redundant. >>> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c >>> b/drivers/net/ethernet/intel/igc/igc_tsn.c >>> index 6b299b83e7ef2..81770955029dc 100644 >>> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c >>> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c >>> @@ -114,7 +114,6 @@ static int igc_tsn_disable_offload(struct igc_adapter >> *adapter) >>> static int igc_tsn_enable_offload(struct igc_adapter *adapter) >>> { >>> struct igc_hw *hw = &adapter->hw; >>> - bool tsn_mode_reconfig = false; >>> u32 tqavctrl, baset_l, baset_h; >>> u32 sec, nsec, cycle; >>> ktime_t base_time, systim; >>> @@ -228,11 +227,10 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) >>> >>> tqavctrl = rd32(IGC_TQAVCTRL) & ~IGC_TQAVCTRL_FUTSCDDIS; >>> >>> - if (tqavctrl & IGC_TQAVCTRL_TRANSMIT_MODE_TSN) >>> - tsn_mode_reconfig = true; >>> - >>> tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV; >>> >>> + adapter->qbv_count++; >>> + >>> cycle = adapter->cycle_time; >>> base_time = adapter->base_time; >>> >>> @@ -250,17 +248,28 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) >>> */ >>> if ((rd32(IGC_BASET_H) || rd32(IGC_BASET_L)) && >>> (adapter->tc_setup_type == TC_SETUP_QDISC_TAPRIO) && >>> - tsn_mode_reconfig) >>> + (adapter->qbv_count > 1)) >>> adapter->qbv_config_change_errors++; >>> } else { >>> - /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit >>> - * has to be configured before the cycle time and base time. >>> - * Tx won't hang if there is a GCL is already running, >>> - * so in this case we don't need to set FutScdDis. >>> - */ >>> - if (igc_is_device_id_i226(hw) && >>> - !(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) >>> - tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; >>> + if (igc_is_device_id_i226(hw)) { >>> + ktime_t adjust_time, expires_time; >>> + >>> + /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit >>> + * has to be configured before the cycle time and base time. >>> + * Tx won't hang if there is a GCL is already running, >> >> Remove second *is*? > > Thanks for noticed it. Will rephase and remove it. > >>> + * so in this case we don't need to set FutScdDis. >>> + */ >>> + if (!(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) >>> + tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; >>> + >>> + nsec = rd32(IGC_SYSTIML); >>> + sec = rd32(IGC_SYSTIMH); >>> + systim = ktime_set(sec, nsec); >>> + >>> + adjust_time = adapter->base_time; >>> + expires_time = ktime_sub_ns(adjust_time, systim); >>> + hrtimer_start(&adapter->hrtimer, expires_time, HRTIMER_MODE_REL); >>> + } >>> } >>> >>> wr32(IGC_TQAVCTRL, tqavctrl); >>> @@ -306,7 +315,11 @@ int igc_tsn_offload_apply(struct igc_adapter *adapter) >>> { >>> struct igc_hw *hw = &adapter->hw; >>> >>> - if (netif_running(adapter->netdev) && igc_is_device_id_i225(hw)) { >>> + /* Per I225/6 HW Design Section 7.5.2.1, transmit mode >>> + * cannot be change dynamically. Require reset the adapter. >> >> change*d* > > Good catch! > > Thanks, > Husaini > >>> + */ >>> + if (netif_running(adapter->netdev) && >>> + (igc_is_device_id_i225(hw) || !adapter->qbv_count)) { >>> schedule_work(&adapter->reset_task); >>> return 0; >>> } >> >> >> Kind regards, >> >> Paul
Dear Paul, > -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: Saturday, 3 June, 2023 1:55 PM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com> > Cc: Choong, Chwee Lin <chwee.lin.choong@intel.com>; intel-wired- > lan@osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; > tee.min.tan@linux.intel.com > Subject: Re: [Intel-wired-lan] [PATCH iwl-net v2] igc: Fix TX Hang issue when > QBV Gate is close > > Dear Muhammad, > > > Am 02.06.23 um 15:26 schrieb Zulkifli, Muhammad Husaini: > > >> -----Original Message----- > >> From: Paul Menzel <pmenzel@molgen.mpg.de> > >> Sent: Friday, 2 June, 2023 8:03 PM > >> Am 02.06.23 um 03:28 schrieb Muhammad Husaini Zulkifli: > > >> There is a small typo in the commit message summary/title: close*d*. > > > > Noted. Already fix this. > > > >>> If a user schedules a Gate Control List (GCL) to close one of the > >>> QBV gates while also transmitting a packet to that closed gate, TX > >>> Hang will be happen. HW would not drop any packet when the gate is > >>> close and keep queueing up in HW TX FIFO until the gate is re-open. > >> > >> 1. close*d* > >> 2. queuing > >> 3. re-opened > > > > Will change accordingly π > > > >> > >>> This patch implement the solution to drop the packet for the closed > >>> gate. > >> > >> implement*s* > > > > Yup > > > >>> This patch will additionally include a reset adapter to perform > >> > >> Maybe: Additionally reset the adapter to β¦ > > > > Sure. I will rephrase it. > > > >>> SW initialization for each 1st Gate Control List (GCL) to avoid hang. > >>> This is due to the HW design, where changing to TSN transmit mode > >>> requires SW initialization. Intel Discrete I225/6 transmit mode > >>> cannot be changed when in dynamic mode according to Software User > >>> Manual Section 7.5.2.1. Subsequent Gate Control List (GCL) > >>> operations will proceed without a reset, as they already in TSN Mode. > >> > >> β¦ they already *are* β¦ > > > > Yup. Thanks! > > > >> > >>> Step to reproduce: > >>> > >>> DUT: > >>> 1) Configure GCL List with certain gate close. > >>> 2) Transmit the packet to close gate. > >> > >> close*d* > > > > Noted. > > > >> > >> Do you have the commands to do what you did? > > > > Sure. I will update the command in commit message in v3. > > > >> > >>> Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading") > >>> Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com> > >>> Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> > >>> Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com> > >>> Signed-off-by: Muhammad Husaini Zulkifli > >>> <muhammad.husaini.zulkifli@intel.com> > >>> > >>> --- > >>> V1 -> V2: Fix conflict and apply to net-queue tree. > >>> --- > >>> --- > >>> drivers/net/ethernet/intel/igc/igc.h | 6 +++ > >>> drivers/net/ethernet/intel/igc/igc_main.c | 57 > +++++++++++++++++++++-- > >>> drivers/net/ethernet/intel/igc/igc_tsn.c | 41 ++++++++++------ > >>> 3 files changed, 87 insertions(+), 17 deletions(-) > >>> > >>> diff --git a/drivers/net/ethernet/intel/igc/igc.h > >>> b/drivers/net/ethernet/intel/igc/igc.h > >>> index 0bbd108f28939..127b248054e55 100644 > >>> --- a/drivers/net/ethernet/intel/igc/igc.h > >>> +++ b/drivers/net/ethernet/intel/igc/igc.h > >>> @@ -13,6 +13,7 @@ > >>> #include <linux/ptp_clock_kernel.h> > >>> #include <linux/timecounter.h> > >>> #include <linux/net_tstamp.h> > >>> +#include <linux/hrtimer.h> > >>> > >>> #include "igc_hw.h" > >>> > >>> @@ -100,6 +101,8 @@ struct igc_ring { > >>> u32 start_time; > >>> u32 end_time; > >>> u32 max_sdu; > >>> + bool oper_gate_closed; > >> > >> What does *oper* refer to? > > > > "Oper" refers to the operating gate, or "current gate" in this case. > > This variable indicate that the current queue gate is closed. > > Maybe add a comment on the same line: > > bool oper_gate_closed; /* Is operating/current queue gate closed? */ > > But maybe itβs common knowledge and can also be ommited. Your decision. I'll make a note of it in V3. π Thanks, Husaini > > > Kind regards, > > Paul > > > >>> + bool admin_gate_closed; > >>> > >>> /* CBS parameters */ > >>> bool cbs_enable; /* indicates if CBS is enabled */ > >>> @@ -159,6 +162,7 @@ struct igc_adapter { > >>> struct timer_list watchdog_timer; > >>> struct timer_list dma_err_timer; > >>> struct timer_list phy_info_timer; > >>> + struct hrtimer hrtimer; > >>> > >>> u32 wol; > >>> u32 en_mng_pt; > >>> @@ -188,6 +192,8 @@ struct igc_adapter { > >>> ktime_t cycle_time; > >>> bool qbv_enable; > >>> u32 qbv_config_change_errors; > >>> + bool qbv_transition; > >>> + int qbv_count; > >> > >> unsigned int? > > > > Will do. > > > >> > >>> /* OS defined structs */ > >>> struct pci_dev *pdev; > >>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c > >>> b/drivers/net/ethernet/intel/igc/igc_main.c > >>> index 9095306323afd..1f95376f9ffda 100644 > >>> --- a/drivers/net/ethernet/intel/igc/igc_main.c > >>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c > >>> @@ -1521,6 +1521,9 @@ static netdev_tx_t igc_xmit_frame_ring(struct > sk_buff *skb, > >>> u8 hdr_len = 0; > >>> int tso = 0; > >>> > >>> + if (adapter->qbv_transition || tx_ring->oper_gate_closed) > >>> + goto out_drop; > >>> + > >>> /* need: 1 descriptor per page * > PAGE_SIZE/IGC_MAX_DATA_PER_TXD, > >>> * + 1 desc for skb_headlen/IGC_MAX_DATA_PER_TXD, > >>> * + 2 desc gap to keep tail from touching head, > >>> @@ -2967,7 +2970,8 @@ static bool igc_clean_tx_irq(struct igc_q_vector > *q_vector, int napi_budget) > >>> (adapter->tx_timeout_factor * HZ)) && > >>> !(rd32(IGC_STATUS) & IGC_STATUS_TXOFF) && > >>> (rd32(IGC_TDH(tx_ring->reg_idx)) != > >>> - readl(tx_ring->tail))) { > >>> + readl(tx_ring->tail)) && > >>> + !tx_ring->oper_gate_closed) { > >>> /* detected Tx unit hang */ > >>> netdev_err(tx_ring->netdev, > >>> "Detected Tx Unit Hang\n" > >>> @@ -6057,6 +6061,8 @@ static int igc_tsn_clear_schedule(struct > igc_adapter *adapter) > >>> adapter->base_time = 0; > >>> adapter->cycle_time = NSEC_PER_SEC; > >>> adapter->qbv_config_change_errors = 0; > >>> + adapter->qbv_transition = false; > >>> + adapter->qbv_count = 0; > >>> > >>> for (i = 0; i < adapter->num_tx_queues; i++) { > >>> struct igc_ring *ring = adapter->tx_ring[i]; @@ -6064,6 > >> +6070,8 @@ > >>> static int igc_tsn_clear_schedule(struct igc_adapter *adapter) > >>> ring->start_time = 0; > >>> ring->end_time = NSEC_PER_SEC; > >>> ring->max_sdu = 0; > >>> + ring->oper_gate_closed = false; > >>> + ring->admin_gate_closed = false; > >>> } > >>> > >>> return 0; > >>> @@ -6075,6 +6083,7 @@ static int igc_save_qbv_schedule(struct > igc_adapter *adapter, > >>> bool queue_configured[IGC_MAX_TX_QUEUES] = { }; > >>> struct igc_hw *hw = &adapter->hw; > >>> u32 start_time = 0, end_time = 0; > >>> + struct timespec64 now; > >>> size_t n; > >>> int i; > >>> > >>> @@ -6095,6 +6104,8 @@ static int igc_save_qbv_schedule(struct > >> igc_adapter *adapter, > >>> adapter->cycle_time = qopt->cycle_time; > >>> adapter->base_time = qopt->base_time; > >>> > >>> + igc_ptp_read(adapter, &now); > >>> + > >>> for (n = 0; n < qopt->num_entries; n++) { > >>> struct tc_taprio_sched_entry *e = &qopt->entries[n]; > >>> > >>> @@ -6129,7 +6140,10 @@ static int igc_save_qbv_schedule(struct > igc_adapter *adapter, > >>> ring->start_time = start_time; > >>> ring->end_time = end_time; > >>> > >>> - queue_configured[i] = true; > >>> + if (ring->start_time >= adapter->cycle_time) > >>> + queue_configured[i] = false; > >>> + else > >>> + queue_configured[i] = true; > >> > >> Maybe: > >> > >> queue_configured[i] = !(ring->start_time >= > >> adapter->cycle_time) > > > > I will maintain the original code for this one . Thanks for the suggestion. > > > >>> } > >>> > >>> start_time += e->interval; > >>> @@ -6139,8 +6153,20 @@ static int igc_save_qbv_schedule(struct > igc_adapter *adapter, > >>> * If not, set the start and end time to be end time. > >>> */ > >>> for (i = 0; i < adapter->num_tx_queues; i++) { > >>> + struct igc_ring *ring = adapter->tx_ring[i]; > >>> + > >>> + if (!is_base_time_past(qopt->base_time, &now)) { > >>> + ring->admin_gate_closed = false; > >>> + } else { > >>> + ring->oper_gate_closed = false; > >>> + ring->admin_gate_closed = false; > >>> + } > >>> + > >>> if (!queue_configured[i]) { > >>> - struct igc_ring *ring = adapter->tx_ring[i]; > >>> + if (!is_base_time_past(qopt->base_time, &now)) > >>> + ring->admin_gate_closed = true; > >>> + else > >>> + ring->oper_gate_closed = true; > >>> > >>> ring->start_time = end_time; > >>> ring->end_time = end_time; > >>> @@ -6466,6 +6492,27 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg) > >>> return value; > >>> } > >>> > >>> +static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer > +*timer) { > >>> + struct igc_adapter *adapter = container_of(timer, struct igc_adapter, > >>> + hrtimer); > >>> + int i; > >> > >> unsigned int? > > > > Absolutely will change it π > > > >>> + > >>> + adapter->qbv_transition = true; > >>> + for (i = 0; i < adapter->num_tx_queues; i++) { > >>> + struct igc_ring *tx_ring = adapter->tx_ring[i]; > >>> + > >>> + if (tx_ring->admin_gate_closed) { > >>> + tx_ring->admin_gate_closed = false; > >>> + tx_ring->oper_gate_closed = true; > >>> + } else { > >>> + tx_ring->oper_gate_closed = false; > >>> + } > >>> + } > >>> + adapter->qbv_transition = false; > >>> + return HRTIMER_NORESTART; > >>> +} > >>> + > >>> /** > >>> * igc_probe - Device Initialization Routine > >>> * @pdev: PCI device information struct @@ -6642,6 +6689,9 @@ > >>> static int igc_probe(struct pci_dev *pdev, > >>> INIT_WORK(&adapter->reset_task, igc_reset_task); > >>> INIT_WORK(&adapter->watchdog_task, igc_watchdog_task); > >>> > >>> + hrtimer_init(&adapter->hrtimer, CLOCK_MONOTONIC, > HRTIMER_MODE_REL); > >>> + adapter->hrtimer.function = &igc_qbv_scheduling_timer; > >>> + > >>> /* Initialize link properties that are user-changeable */ > >>> adapter->fc_autoneg = true; > >>> hw->mac.autoneg = true; > >>> @@ -6745,6 +6795,7 @@ static void igc_remove(struct pci_dev *pdev) > >>> > >>> cancel_work_sync(&adapter->reset_task); > >>> cancel_work_sync(&adapter->watchdog_task); > >>> + hrtimer_cancel(&adapter->hrtimer); > >>> > >>> /* Release control of h/w to f/w. If f/w is AMT enabled, this > >>> * would have already happened in close and is redundant. > >>> diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c > >>> b/drivers/net/ethernet/intel/igc/igc_tsn.c > >>> index 6b299b83e7ef2..81770955029dc 100644 > >>> --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > >>> +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > >>> @@ -114,7 +114,6 @@ static int igc_tsn_disable_offload(struct > >>> igc_adapter > >> *adapter) > >>> static int igc_tsn_enable_offload(struct igc_adapter *adapter) > >>> { > >>> struct igc_hw *hw = &adapter->hw; > >>> - bool tsn_mode_reconfig = false; > >>> u32 tqavctrl, baset_l, baset_h; > >>> u32 sec, nsec, cycle; > >>> ktime_t base_time, systim; > >>> @@ -228,11 +227,10 @@ static int igc_tsn_enable_offload(struct > >>> igc_adapter *adapter) > >>> > >>> tqavctrl = rd32(IGC_TQAVCTRL) & ~IGC_TQAVCTRL_FUTSCDDIS; > >>> > >>> - if (tqavctrl & IGC_TQAVCTRL_TRANSMIT_MODE_TSN) > >>> - tsn_mode_reconfig = true; > >>> - > >>> tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | > >>> IGC_TQAVCTRL_ENHANCED_QAV; > >>> > >>> + adapter->qbv_count++; > >>> + > >>> cycle = adapter->cycle_time; > >>> base_time = adapter->base_time; > >>> > >>> @@ -250,17 +248,28 @@ static int igc_tsn_enable_offload(struct > igc_adapter *adapter) > >>> */ > >>> if ((rd32(IGC_BASET_H) || rd32(IGC_BASET_L)) && > >>> (adapter->tc_setup_type == TC_SETUP_QDISC_TAPRIO) && > >>> - tsn_mode_reconfig) > >>> + (adapter->qbv_count > 1)) > >>> adapter->qbv_config_change_errors++; > >>> } else { > >>> - /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit > >>> - * has to be configured before the cycle time and base time. > >>> - * Tx won't hang if there is a GCL is already running, > >>> - * so in this case we don't need to set FutScdDis. > >>> - */ > >>> - if (igc_is_device_id_i226(hw) && > >>> - !(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) > >>> - tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; > >>> + if (igc_is_device_id_i226(hw)) { > >>> + ktime_t adjust_time, expires_time; > >>> + > >>> + /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit > >>> + * has to be configured before the cycle time and base > time. > >>> + * Tx won't hang if there is a GCL is already running, > >> > >> Remove second *is*? > > > > Thanks for noticed it. Will rephase and remove it. > > > >>> + * so in this case we don't need to set FutScdDis. > >>> + */ > >>> + if (!(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) > >>> + tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; > >>> + > >>> + nsec = rd32(IGC_SYSTIML); > >>> + sec = rd32(IGC_SYSTIMH); > >>> + systim = ktime_set(sec, nsec); > >>> + > >>> + adjust_time = adapter->base_time; > >>> + expires_time = ktime_sub_ns(adjust_time, systim); > >>> + hrtimer_start(&adapter->hrtimer, expires_time, > HRTIMER_MODE_REL); > >>> + } > >>> } > >>> > >>> wr32(IGC_TQAVCTRL, tqavctrl); > >>> @@ -306,7 +315,11 @@ int igc_tsn_offload_apply(struct igc_adapter > *adapter) > >>> { > >>> struct igc_hw *hw = &adapter->hw; > >>> > >>> - if (netif_running(adapter->netdev) && igc_is_device_id_i225(hw)) { > >>> + /* Per I225/6 HW Design Section 7.5.2.1, transmit mode > >>> + * cannot be change dynamically. Require reset the adapter. > >> > >> change*d* > > > > Good catch! > > > > Thanks, > > Husaini > > > >>> + */ > >>> + if (netif_running(adapter->netdev) && > >>> + (igc_is_device_id_i225(hw) || !adapter->qbv_count)) { > >>> schedule_work(&adapter->reset_task); > >>> return 0; > >>> } > >> > >> > >> Kind regards, > >> > >> Paul
Dear Anthony, > -----Original Message----- > From: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Sent: Saturday, 3 June, 2023 1:46 AM > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; intel- > wired-lan@osuosl.org > Cc: Gomes, Vinicius <vinicius.gomes@intel.com>; Neftin, Sasha > <sasha.neftin@intel.com>; tee.min.tan@linux.intel.com; Choong, Chwee Lin > <chwee.lin.choong@intel.com>; naamax.meir@linux.intel.com > Subject: Re: [PATCH iwl-net v2] igc: Fix TX Hang issue when QBV Gate is close > > > > On 6/1/2023 6:28 PM, Muhammad Husaini Zulkifli wrote: > > If a user schedules a Gate Control List (GCL) to close one of the QBV > > gates while also transmitting a packet to that closed gate, TX Hang > > will be happen. HW would not drop any packet when the gate is close > > and keep queueing up in HW TX FIFO until the gate is re-open. > > This patch implement the solution to drop the packet for the closed > > gate. > > > > This patch will additionally include a reset adapter to perform SW > > initialization for each 1st Gate Control List (GCL) to avoid hang. > > This is due to the HW design, where changing to TSN transmit mode > > requires SW initialization. Intel Discrete I225/6 transmit mode cannot > > be changed when in dynamic mode according to Software User Manual > > Section 7.5.2.1. Subsequent Gate Control List (GCL) operations will > > proceed without a reset, as they already in TSN Mode. > > > > Step to reproduce: > > > > DUT: > > 1) Configure GCL List with certain gate close. > > 2) Transmit the packet to close gate. > > > > Fixes: ec50a9d437f0 ("igc: Add support for taprio offloading") > > Co-developed-by: Tan Tee Min <tee.min.tan@linux.intel.com> > > Signed-off-by: Tan Tee Min <tee.min.tan@linux.intel.com> > > Tested-by: Chwee Lin Choong <chwee.lin.choong@intel.com> > > Signed-off-by: Muhammad Husaini Zulkifli > > <muhammad.husaini.zulkifli@intel.com> > > > > No newline here please. Ops. OK will remove it. > > > --- > > ... > > > V1 -> V2: Fix conflict and apply to net-queue tree. > > --- > > --- > > no need for two '---' Yup. Will remove this double line. > > > @@ -1521,6 +1521,9 @@ static netdev_tx_t igc_xmit_frame_ring(struct > sk_buff *skb, > > u8 hdr_len = 0; > > int tso = 0; > > > > + if (adapter->qbv_transition || tx_ring->oper_gate_closed) > > + goto out_drop; > > + > > clang reports issues with this: > > +../drivers/net/ethernet/intel/igc/igc_main.c:1524:6: warning: variable > 'first' is used uninitialized whenever 'if' condition is true [-Wsometimes- > uninitialized] > + if (adapter->qbv_transition || tx_ring->oper_gate_closed) > + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > +../drivers/net/ethernet/intel/igc/igc_main.c:1632:20: note: > uninitialized use occurs here > + dev_kfree_skb_any(first->skb); > + ^~~~~ > +../drivers/net/ethernet/intel/igc/igc_main.c:1524:2: note: remove the > 'if' if its condition is always false > + if (adapter->qbv_transition || tx_ring->oper_gate_closed) > + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > +../drivers/net/ethernet/intel/igc/igc_main.c:1524:6: warning: variable > 'first' is used uninitialized whenever '||' condition is true [-Wsometimes- > uninitialized] > + if (adapter->qbv_transition || tx_ring->oper_gate_closed) > + ^~~~~~~~~~~~~~~~~~~~~~~ > +../drivers/net/ethernet/intel/igc/igc_main.c:1632:20: note: > uninitialized use occurs here > + dev_kfree_skb_any(first->skb); > + ^~~~~ > +../drivers/net/ethernet/intel/igc/igc_main.c:1524:6: note: remove the > '||' if its condition is always false > + if (adapter->qbv_transition || tx_ring->oper_gate_closed) > + ^~~~~~~~~~~~~~~~~~~~~~~~~~ > +../drivers/net/ethernet/intel/igc/igc_main.c:1516:29: note: initialize > the variable 'first' to silence this warning > + struct igc_tx_buffer *first; > + ^ > + = NULL > +2 warnings generated. I will move "if (adapter->qbv_transition || tx_ring->oper_gate_closed)" to Line 1575 below to avoid this warning. My environment is unable to run CC=clang. But I believe this will resolve the issue. Thanks, Husaini > > > > /* need: 1 descriptor per page * > PAGE_SIZE/IGC_MAX_DATA_PER_TXD, > > * + 1 desc for skb_headlen/IGC_MAX_DATA_PER_TXD, > > * + 2 desc gap to keep tail from touching head,
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index 0bbd108f28939..127b248054e55 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -13,6 +13,7 @@ #include <linux/ptp_clock_kernel.h> #include <linux/timecounter.h> #include <linux/net_tstamp.h> +#include <linux/hrtimer.h> #include "igc_hw.h" @@ -100,6 +101,8 @@ struct igc_ring { u32 start_time; u32 end_time; u32 max_sdu; + bool oper_gate_closed; + bool admin_gate_closed; /* CBS parameters */ bool cbs_enable; /* indicates if CBS is enabled */ @@ -159,6 +162,7 @@ struct igc_adapter { struct timer_list watchdog_timer; struct timer_list dma_err_timer; struct timer_list phy_info_timer; + struct hrtimer hrtimer; u32 wol; u32 en_mng_pt; @@ -188,6 +192,8 @@ struct igc_adapter { ktime_t cycle_time; bool qbv_enable; u32 qbv_config_change_errors; + bool qbv_transition; + int qbv_count; /* OS defined structs */ struct pci_dev *pdev; diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 9095306323afd..1f95376f9ffda 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -1521,6 +1521,9 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb, u8 hdr_len = 0; int tso = 0; + if (adapter->qbv_transition || tx_ring->oper_gate_closed) + goto out_drop; + /* need: 1 descriptor per page * PAGE_SIZE/IGC_MAX_DATA_PER_TXD, * + 1 desc for skb_headlen/IGC_MAX_DATA_PER_TXD, * + 2 desc gap to keep tail from touching head, @@ -2967,7 +2970,8 @@ static bool igc_clean_tx_irq(struct igc_q_vector *q_vector, int napi_budget) (adapter->tx_timeout_factor * HZ)) && !(rd32(IGC_STATUS) & IGC_STATUS_TXOFF) && (rd32(IGC_TDH(tx_ring->reg_idx)) != - readl(tx_ring->tail))) { + readl(tx_ring->tail)) && + !tx_ring->oper_gate_closed) { /* detected Tx unit hang */ netdev_err(tx_ring->netdev, "Detected Tx Unit Hang\n" @@ -6057,6 +6061,8 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter) adapter->base_time = 0; adapter->cycle_time = NSEC_PER_SEC; adapter->qbv_config_change_errors = 0; + adapter->qbv_transition = false; + adapter->qbv_count = 0; for (i = 0; i < adapter->num_tx_queues; i++) { struct igc_ring *ring = adapter->tx_ring[i]; @@ -6064,6 +6070,8 @@ static int igc_tsn_clear_schedule(struct igc_adapter *adapter) ring->start_time = 0; ring->end_time = NSEC_PER_SEC; ring->max_sdu = 0; + ring->oper_gate_closed = false; + ring->admin_gate_closed = false; } return 0; @@ -6075,6 +6083,7 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, bool queue_configured[IGC_MAX_TX_QUEUES] = { }; struct igc_hw *hw = &adapter->hw; u32 start_time = 0, end_time = 0; + struct timespec64 now; size_t n; int i; @@ -6095,6 +6104,8 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, adapter->cycle_time = qopt->cycle_time; adapter->base_time = qopt->base_time; + igc_ptp_read(adapter, &now); + for (n = 0; n < qopt->num_entries; n++) { struct tc_taprio_sched_entry *e = &qopt->entries[n]; @@ -6129,7 +6140,10 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, ring->start_time = start_time; ring->end_time = end_time; - queue_configured[i] = true; + if (ring->start_time >= adapter->cycle_time) + queue_configured[i] = false; + else + queue_configured[i] = true; } start_time += e->interval; @@ -6139,8 +6153,20 @@ static int igc_save_qbv_schedule(struct igc_adapter *adapter, * If not, set the start and end time to be end time. */ for (i = 0; i < adapter->num_tx_queues; i++) { + struct igc_ring *ring = adapter->tx_ring[i]; + + if (!is_base_time_past(qopt->base_time, &now)) { + ring->admin_gate_closed = false; + } else { + ring->oper_gate_closed = false; + ring->admin_gate_closed = false; + } + if (!queue_configured[i]) { - struct igc_ring *ring = adapter->tx_ring[i]; + if (!is_base_time_past(qopt->base_time, &now)) + ring->admin_gate_closed = true; + else + ring->oper_gate_closed = true; ring->start_time = end_time; ring->end_time = end_time; @@ -6466,6 +6492,27 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg) return value; } +static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer) +{ + struct igc_adapter *adapter = container_of(timer, struct igc_adapter, + hrtimer); + int i; + + adapter->qbv_transition = true; + for (i = 0; i < adapter->num_tx_queues; i++) { + struct igc_ring *tx_ring = adapter->tx_ring[i]; + + if (tx_ring->admin_gate_closed) { + tx_ring->admin_gate_closed = false; + tx_ring->oper_gate_closed = true; + } else { + tx_ring->oper_gate_closed = false; + } + } + adapter->qbv_transition = false; + return HRTIMER_NORESTART; +} + /** * igc_probe - Device Initialization Routine * @pdev: PCI device information struct @@ -6642,6 +6689,9 @@ static int igc_probe(struct pci_dev *pdev, INIT_WORK(&adapter->reset_task, igc_reset_task); INIT_WORK(&adapter->watchdog_task, igc_watchdog_task); + hrtimer_init(&adapter->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + adapter->hrtimer.function = &igc_qbv_scheduling_timer; + /* Initialize link properties that are user-changeable */ adapter->fc_autoneg = true; hw->mac.autoneg = true; @@ -6745,6 +6795,7 @@ static void igc_remove(struct pci_dev *pdev) cancel_work_sync(&adapter->reset_task); cancel_work_sync(&adapter->watchdog_task); + hrtimer_cancel(&adapter->hrtimer); /* Release control of h/w to f/w. If f/w is AMT enabled, this * would have already happened in close and is redundant. diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index 6b299b83e7ef2..81770955029dc 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.c +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c @@ -114,7 +114,6 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter) static int igc_tsn_enable_offload(struct igc_adapter *adapter) { struct igc_hw *hw = &adapter->hw; - bool tsn_mode_reconfig = false; u32 tqavctrl, baset_l, baset_h; u32 sec, nsec, cycle; ktime_t base_time, systim; @@ -228,11 +227,10 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) tqavctrl = rd32(IGC_TQAVCTRL) & ~IGC_TQAVCTRL_FUTSCDDIS; - if (tqavctrl & IGC_TQAVCTRL_TRANSMIT_MODE_TSN) - tsn_mode_reconfig = true; - tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN | IGC_TQAVCTRL_ENHANCED_QAV; + adapter->qbv_count++; + cycle = adapter->cycle_time; base_time = adapter->base_time; @@ -250,17 +248,28 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) */ if ((rd32(IGC_BASET_H) || rd32(IGC_BASET_L)) && (adapter->tc_setup_type == TC_SETUP_QDISC_TAPRIO) && - tsn_mode_reconfig) + (adapter->qbv_count > 1)) adapter->qbv_config_change_errors++; } else { - /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit - * has to be configured before the cycle time and base time. - * Tx won't hang if there is a GCL is already running, - * so in this case we don't need to set FutScdDis. - */ - if (igc_is_device_id_i226(hw) && - !(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) - tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; + if (igc_is_device_id_i226(hw)) { + ktime_t adjust_time, expires_time; + + /* According to datasheet section 7.5.2.9.3.3, FutScdDis bit + * has to be configured before the cycle time and base time. + * Tx won't hang if there is a GCL is already running, + * so in this case we don't need to set FutScdDis. + */ + if (!(rd32(IGC_BASET_H) || rd32(IGC_BASET_L))) + tqavctrl |= IGC_TQAVCTRL_FUTSCDDIS; + + nsec = rd32(IGC_SYSTIML); + sec = rd32(IGC_SYSTIMH); + systim = ktime_set(sec, nsec); + + adjust_time = adapter->base_time; + expires_time = ktime_sub_ns(adjust_time, systim); + hrtimer_start(&adapter->hrtimer, expires_time, HRTIMER_MODE_REL); + } } wr32(IGC_TQAVCTRL, tqavctrl); @@ -306,7 +315,11 @@ int igc_tsn_offload_apply(struct igc_adapter *adapter) { struct igc_hw *hw = &adapter->hw; - if (netif_running(adapter->netdev) && igc_is_device_id_i225(hw)) { + /* Per I225/6 HW Design Section 7.5.2.1, transmit mode + * cannot be change dynamically. Require reset the adapter. + */ + if (netif_running(adapter->netdev) && + (igc_is_device_id_i225(hw) || !adapter->qbv_count)) { schedule_work(&adapter->reset_task); return 0; }