From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6F14DA0471 for ; Tue, 16 Jul 2019 03:05:45 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7DAFF2C5E; Tue, 16 Jul 2019 03:05:44 +0200 (CEST) Received: from mail-io1-f66.google.com (mail-io1-f66.google.com [209.85.166.66]) by dpdk.org (Postfix) with ESMTP id D2C7C2C23 for ; Tue, 16 Jul 2019 03:05:42 +0200 (CEST) Received: by mail-io1-f66.google.com with SMTP id f4so37237940ioh.6 for ; Mon, 15 Jul 2019 18:05:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Z88EIJoREzHkSujw5t28bkRVQ5NYM/7RBUi2b+pnk/M=; b=d0z1qvYA2PCthDXleeqLWLeqBYeqEPCHV4e8eUDSIn1hHuhcRysvrjT3Vb3JWXho8L +1ziqOTz49pddR1eqJHOjuYQoB5rMGqDPmh8m/QnLa2cOrR2zv6s1XdsBsptp/Nq2M7h 5Eezu1Hla3AML7BkpRDTFw10WwRj9xoftuLEFHgOVgrT6bISVKpBg8AlrtTKa8nNewhx XkHHhLzXotXgWkQCzcS4bXDNGhl+uOMXqKCc/AdaML2zncPSJ+ZRmF+rZSy/JAtn5fMi pu7MlmTAsAMxOlpZ9WmcQ8F7Gl9LxlOB7Zgt7pnuCmLq3TyJkfr9PzFJEj/9G3Lf/IMQ C8RQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Z88EIJoREzHkSujw5t28bkRVQ5NYM/7RBUi2b+pnk/M=; b=Z2x4cAXFgTuIfSNVJZC34Ejyx8JF3OPv1/8JOPTi3T5cISP1iAAJ0HgvXIoJi9H80k WyMlcidy992eldJzXTUi6FSKEbl2wGD+k1jb3vLee5dBhv5dWmL8/vI6q7gCgGDNId0I D0qjZYbRNVkJ2t1eK2r6rLjg06tIraJWnyXFSrJFUiANKwYHeB3MVWNrNaS1VSMzfpDl HIPwCcWbn5cHL3GxneifUGGQGOB+1EqZklZEVpE8LH4yeLklhMpBCrw7uZcvYHvtf7bz hdyEKw1ctI952Z+PwpLAx1TZ4hYzdgpBJYAGz3jCufFH7u048oDfT3JvfwAFGKH0air9 Ny5w== X-Gm-Message-State: APjAAAUGGBBiA1Uhpin7XQZZM79jsXGqCnKaFEsghiMKy3XK5JHmfyNe IbNc8G98qr0Fl1B7naEyzVZXQniyAF/9WGzc9KY= X-Google-Smtp-Source: APXvYqy8+9Ao291m/hzkx50Bb2bmdATMCGxqUoXBmVw7xkE2Isi+E0/hRWtERMB1JtWCqJuhdSV2dyYDdqaWjSxmFbU= X-Received: by 2002:a02:7a5c:: with SMTP id z28mr31120740jad.40.1563239142134; Mon, 15 Jul 2019 18:05:42 -0700 (PDT) MIME-Version: 1.0 References: <1562719355-17592-1-git-send-email-xiao.zhang@intel.com> In-Reply-To: From: "Anand H. Krishnan" Date: Tue, 16 Jul 2019 06:33:28 +0530 Message-ID: To: Xiao Zhang Cc: dev@dpdk.org, "Zhao1, Wei" Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [v4] net/e1000: i219 unit hang issue fix on reset/close X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Xiao, I didn't hear back from you on the comments. Thanks, Anand On Wed, Jul 10, 2019 at 10:18 AM Anand H. Krishnan wrote: > > More comments inline: > > On Tue, Jul 9, 2019 at 9:16 PM Xiao Zhang wrote: > > > > Unit hang may occur if multiple descriptors are available in the rings > > during reset or close. This state can be detected by configure status > > by bit 8 in register. If the bit is set and there are pending descriptors > > in one of the rings, we must flush them before reset or close. > > > > Signed-off-by: Xiao Zhang > > --- > > drivers/net/e1000/e1000_ethdev.h | 4 ++ > > drivers/net/e1000/igb_ethdev.c | 4 ++ > > drivers/net/e1000/igb_rxtx.c | 105 +++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 113 insertions(+) > > > > diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h > > index 67acb73..349144a 100644 > > --- a/drivers/net/e1000/e1000_ethdev.h > > +++ b/drivers/net/e1000/e1000_ethdev.h > > @@ -35,6 +35,9 @@ > > #define IGB_MAX_RX_QUEUE_NUM 8 > > #define IGB_MAX_RX_QUEUE_NUM_82576 16 > > > > +#define E1000_I219_MAX_RX_QUEUE_NUM 2 > > +#define E1000_I219_MAX_TX_QUEUE_NUM 2 > > + > > #define E1000_SYN_FILTER_ENABLE 0x00000001 /* syn filter enable field */ > > #define E1000_SYN_FILTER_QUEUE 0x0000000E /* syn filter queue field */ > > #define E1000_SYN_FILTER_QUEUE_SHIFT 1 /* syn filter queue field */ > > @@ -522,5 +525,6 @@ int igb_action_rss_same(const struct rte_flow_action_rss *comp, > > int igb_config_rss_filter(struct rte_eth_dev *dev, > > struct igb_rte_flow_rss_conf *conf, > > bool add); > > +void igb_flush_desc_rings(struct rte_eth_dev *dev); > > > > #endif /* _E1000_ETHDEV_H_ */ > > diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c > > index 3ee28cf..845101b 100644 > > --- a/drivers/net/e1000/igb_ethdev.c > > +++ b/drivers/net/e1000/igb_ethdev.c > > @@ -1589,6 +1589,10 @@ eth_igb_close(struct rte_eth_dev *dev) > > eth_igb_stop(dev); > > adapter->stopped = 1; > > > > + /* Flush desc rings for i219 */ > > + if (hw->mac.type >= e1000_pch_spt) > > + igb_flush_desc_rings(dev); > > + > > I am a bit confused as to which driver handles i219. I thought it is > the em_ethdev.c. > Also, the place to put this code, I think is more appropriate in eth_em_stop. > > > > e1000_phy_hw_reset(hw); > > igb_release_manageability(hw); > > igb_hw_control_release(hw); > > diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c > > index c5606de..cad28f3 100644 > > --- a/drivers/net/e1000/igb_rxtx.c > > +++ b/drivers/net/e1000/igb_rxtx.c > > @@ -18,6 +18,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -63,6 +64,10 @@ > > #define IGB_TX_OFFLOAD_NOTSUP_MASK \ > > (PKT_TX_OFFLOAD_MASK ^ IGB_TX_OFFLOAD_MASK) > > > > +/* PCI offset for querying descriptor ring status*/ > > +#define PCICFG_DESC_RING_STATUS 0xE4 > > +#define FLUSH_DESC_REQUIRED 0x100 > > + > > /** > > * Structure associated with each descriptor of the RX ring of a RX queue. > > */ > > @@ -2962,3 +2967,103 @@ igb_config_rss_filter(struct rte_eth_dev *dev, > > > > return 0; > > } > > + > > +static void e1000_flush_tx_ring(struct rte_eth_dev *dev) > > +{ > > + struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > + volatile union e1000_adv_tx_desc *tx_desc; > > + uint32_t tdt, tctl, txd_lower = E1000_TXD_CMD_IFCS; > > + uint16_t size = 512; > > + struct igb_tx_queue *txq; > > + int i; > > + > > + if (dev->data->tx_queues == NULL) > > + return; > > + tctl = E1000_READ_REG(hw, E1000_TCTL); > > + E1000_WRITE_REG(hw, E1000_TCTL, tctl | E1000_TCTL_EN); > > + for (i = 0; i < dev->data->nb_tx_queues && > > + i < E1000_I219_MAX_TX_QUEUE_NUM; i++) { > > + txq = dev->data->tx_queues[i]; > > + tdt = E1000_READ_REG(hw, E1000_TDT(i)); > > + if (tdt != txq->tx_tail) > > + return; > > + tx_desc = &txq->tx_ring[txq->tx_tail]; > > + tx_desc->read.buffer_addr = txq->tx_ring_phys_addr; > > Should you be using rte_cpu_to_le64 here? > > > > + tx_desc->read.cmd_type_len = rte_cpu_to_le_32(txd_lower | size); > > There is a lower.data and upper.data that needs to be set, not this > field AFAIR. > > > > + tx_desc->read.olinfo_status = 0; > > + > > + rte_wmb(); > > + txq->tx_tail++; > > + if (txq->tx_tail == txq->nb_tx_desc) > > + txq->tx_tail = 0; > > + rte_io_wmb(); > > + E1000_WRITE_REG(hw, E1000_TDT(i), txq->tx_tail); > > Should you be using E1000_PCI_REG_WRITE_RELAXED. Can you just check > other instances of how this register is written? > > Thanks, > Anand > > > > + usec_delay(250); > > + } > > +} > > + > > +static void e1000_flush_rx_ring(struct rte_eth_dev *dev) > > +{ > > + uint32_t rctl, rxdctl; > > + struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > + int i; > > + > > + rctl = E1000_READ_REG(hw, E1000_RCTL); > > + E1000_WRITE_REG(hw, E1000_RCTL, rctl & ~E1000_RCTL_EN); > > + E1000_WRITE_FLUSH(hw); > > + usec_delay(150); > > + > > + for (i = 0; i < dev->data->nb_rx_queues && > > + i < E1000_I219_MAX_RX_QUEUE_NUM; i++) { > > + rxdctl = E1000_READ_REG(hw, E1000_RXDCTL(i)); > > + /* zero the lower 14 bits (prefetch and host thresholds) */ > > + rxdctl &= 0xffffc000; > > + > > + /* update thresholds: prefetch threshold to 31, > > + * host threshold to 1 and make sure the granularity > > + * is "descriptors" and not "cache lines" > > + */ > > + rxdctl |= (0x1F | (1UL << 8) | E1000_RXDCTL_THRESH_UNIT_DESC); > > + > > + E1000_WRITE_REG(hw, E1000_RXDCTL(i), rxdctl); > > + } > > + /* momentarily enable the RX ring for the changes to take effect */ > > + E1000_WRITE_REG(hw, E1000_RCTL, rctl | E1000_RCTL_EN); > > + E1000_WRITE_FLUSH(hw); > > + usec_delay(150); > > + E1000_WRITE_REG(hw, E1000_RCTL, rctl & ~E1000_RCTL_EN); > > +} > > + > > +/** > > + * igb_flush_desc_rings - remove all descriptors from the descriptor rings > > + * > > + * In i219, the descriptor rings must be emptied before resetting/closing the > > + * HW. Failure to do this will cause the HW to enter a unit hang state which > > + * can only be released by PCI reset on the device > > + * > > + */ > > + > > +void igb_flush_desc_rings(struct rte_eth_dev *dev) > > +{ > > + uint32_t fextnvm11, tdlen; > > + struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > + struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); > > + uint32_t hang_state = 0; > > + > > + fextnvm11 = E1000_READ_REG(hw, E1000_FEXTNVM11); > > + E1000_WRITE_REG(hw, E1000_FEXTNVM11, > > + fextnvm11 | E1000_FEXTNVM11_DISABLE_MULR_FIX); > > + tdlen = E1000_READ_REG(hw, E1000_TDLEN(0)); > > + rte_pci_read_config(pci_dev, &hang_state, sizeof(hang_state), > > + PCICFG_DESC_RING_STATUS); > > + > > + /* do nothing if we're not in faulty state, or if the queue is empty */ > > + if ((hang_state & FLUSH_DESC_REQUIRED) && tdlen) { > > + /* flush desc ring */ > > + e1000_flush_tx_ring(dev); > > + rte_pci_read_config(pci_dev, &hang_state, sizeof(hang_state), > > + PCICFG_DESC_RING_STATUS); > > + if (hang_state & FLUSH_DESC_REQUIRED) > > + e1000_flush_rx_ring(dev); > > + } > > +} > > -- > > 2.7.4 > >