| 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; }
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> --- 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(-)