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 84F55A00C3; Thu, 14 May 2020 09:52:23 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5D4FF1D678; Thu, 14 May 2020 09:52:23 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 825CD1D676; Thu, 14 May 2020 09:52:21 +0200 (CEST) IronPort-SDR: CFy79iLy7cFUyWGQ0CSJjl5/fmgxtHaMtAsKLRryd5+AS/x/1IqmB7OCYMid4nPun3teFCUY42 6d3nKmmt8YOg== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 May 2020 00:52:20 -0700 IronPort-SDR: 8j2jxv2PSM11dAEjxcDWC9t6z/qQTfssMm+MLpit9QKrid2R0oV9xn3yf9VrvnQrCWs6pha+gL aKdMk20wq0kA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,390,1583222400"; d="scan'208";a="287322395" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.116.183]) by fmsmga004.fm.intel.com with ESMTP; 14 May 2020 00:52:18 -0700 Date: Thu, 14 May 2020 15:44:04 +0800 From: Ye Xiaolong To: Qiming Yang Cc: dev@dpdk.org, beilei.xing@intel.com, stable@dpdk.org Message-ID: <20200514074404.GC102809@intel.com> References: <20200513075630.98139-1-qiming.yang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200513075630.98139-1-qiming.yang@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH] net/i40e: fix queue related exception handling 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" On 05/13, Qiming Yang wrote: >There should have different behavior in queue start fail and stop fail case. >When queue start fail, all the next actions should be terminated and then >started queues should be cleared. But for queue stop stage, one queue stop >fail should not end other queues stop. This patch fixed that issue in PF >and VF. > >Fixes: b6583ee40265 ("i40e: full VMDQ pools support") >Fixes: 3f6a696f1054 ("i40evf: queue start and stop") > >Signed-off-by: Qiming Yang >--- > drivers/net/i40e/i40e_ethdev.c | 116 ++++++++------------------------------ > drivers/net/i40e/i40e_ethdev_vf.c | 2 - > drivers/net/i40e/i40e_rxtx.c | 28 +++++++++ > 3 files changed, 53 insertions(+), 93 deletions(-) > >diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c >index 9fbda1c..024178d 100644 >--- a/drivers/net/i40e/i40e_ethdev.c >+++ b/drivers/net/i40e/i40e_ethdev.c >@@ -2276,6 +2276,7 @@ i40e_dev_start(struct rte_eth_dev *dev) > struct rte_intr_handle *intr_handle = &pci_dev->intr_handle; > uint32_t intr_vector = 0; > struct i40e_vsi *vsi; >+ uint16_t nb_rxq, nb_txq; > > hw->adapter_stopped = 0; > >@@ -2307,7 +2308,7 @@ i40e_dev_start(struct rte_eth_dev *dev) > ret = i40e_dev_rxtx_init(pf); > if (ret != I40E_SUCCESS) { > PMD_DRV_LOG(ERR, "Failed to init rx/tx queues"); >- goto err_up; >+ return ret; > } > > /* Map queues with MSIX interrupt */ >@@ -2332,10 +2333,16 @@ i40e_dev_start(struct rte_eth_dev *dev) > } > > /* Enable all queues which have been configured */ >- ret = i40e_dev_switch_queues(pf, TRUE); >- if (ret != I40E_SUCCESS) { >- PMD_DRV_LOG(ERR, "Failed to enable VSI"); >- goto err_up; >+ for (nb_rxq = 0; nb_rxq < dev->data->nb_rx_queues; nb_rxq++) { >+ ret = i40e_dev_rx_queue_start(dev, nb_rxq); >+ if (ret) >+ goto rx_err; >+ } >+ >+ for (nb_txq = 0; nb_txq < dev->data->nb_tx_queues; nb_txq++) { >+ ret = i40e_dev_tx_queue_start(dev, nb_txq); >+ if (ret) >+ goto tx_err; > } > > /* Enable receiving broadcast packets */ >@@ -2365,7 +2372,7 @@ i40e_dev_start(struct rte_eth_dev *dev) > ret = i40e_aq_set_lb_modes(hw, dev->data->dev_conf.lpbk_mode, NULL); > if (ret != I40E_SUCCESS) { > PMD_DRV_LOG(ERR, "fail to set loopback link"); >- goto err_up; >+ goto tx_err; > } > } > >@@ -2373,7 +2380,7 @@ i40e_dev_start(struct rte_eth_dev *dev) > ret = i40e_apply_link_speed(dev); > if (I40E_SUCCESS != ret) { > PMD_DRV_LOG(ERR, "Fail to apply link setting"); >- goto err_up; >+ goto tx_err; > } > > if (!rte_intr_allow_others(intr_handle)) { >@@ -2416,9 +2423,12 @@ i40e_dev_start(struct rte_eth_dev *dev) > > return I40E_SUCCESS; > >-err_up: >- i40e_dev_switch_queues(pf, FALSE); >- i40e_dev_clear_queues(dev); >+tx_err: >+ for (i = 0; i < nb_txq; i++) >+ i40e_dev_tx_queue_stop(dev, i); >+rx_err: >+ for (i = 0; i < nb_rxq; i++) >+ i40e_dev_rx_queue_stop(dev, i); I think we still need to clear queues in the error handling. > > return ret; > } >@@ -2442,7 +2452,11 @@ i40e_dev_stop(struct rte_eth_dev *dev) > } > > /* Disable all queues */ >- i40e_dev_switch_queues(pf, FALSE); >+ for (i = 0; i < dev->data->nb_tx_queues; i++) >+ i40e_dev_tx_queue_stop(dev, i); >+ >+ for (i = 0; i < dev->data->nb_rx_queues; i++) >+ i40e_dev_rx_queue_stop(dev, i); > > /* un-map queues with interrupt registers */ > i40e_vsi_disable_queues_intr(main_vsi); [snip] >diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c >index f6d23c9..d0bada9 100644 >--- a/drivers/net/i40e/i40e_rxtx.c >+++ b/drivers/net/i40e/i40e_rxtx.c >@@ -1570,6 +1570,15 @@ i40e_dev_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id) > PMD_INIT_FUNC_TRACE(); > > rxq = dev->data->rx_queues[rx_queue_id]; >+ if (!rxq || !rxq->q_set) { >+ PMD_DRV_LOG(ERR, "RX queue %u not available or setup", >+ rx_queue_id); >+ return -EINVAL; >+ } >+ >+ if (rxq->rx_deferred_start) >+ PMD_DRV_LOG(ERR, "RX queue %u is deferrd start", >+ rx_queue_id); Do we need to take any action if rx_deferred_start is set? Just print an ERR log doesn't make sense to me. > > err = i40e_alloc_rx_queue_mbufs(rxq); > if (err) { >@@ -1602,6 +1611,11 @@ i40e_dev_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rx_queue_id) > struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > rxq = dev->data->rx_queues[rx_queue_id]; >+ if (!rxq || !rxq->q_set) { >+ PMD_DRV_LOG(ERR, "RX queue %u not available or setup", >+ rx_queue_id); >+ return -EINVAL; >+ } > > /* > * rx_queue_id is queue id application refers to, while >@@ -1630,6 +1644,15 @@ i40e_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id) > PMD_INIT_FUNC_TRACE(); > > txq = dev->data->tx_queues[tx_queue_id]; >+ if (!txq || !txq->q_set) { >+ PMD_DRV_LOG(ERR, "TX queue %u is not available or setup", >+ tx_queue_id); >+ return -EINVAL; >+ } >+ >+ if (txq->tx_deferred_start) >+ PMD_DRV_LOG(ERR, "TX queue %u is deferrd start", >+ tx_queue_id); Ditto. Thanks, Xiaolong > > /* > * tx_queue_id is queue id application refers to, while >@@ -1654,6 +1677,11 @@ i40e_dev_tx_queue_stop(struct rte_eth_dev *dev, uint16_t tx_queue_id) > struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > txq = dev->data->tx_queues[tx_queue_id]; >+ if (!txq || !txq->q_set) { >+ PMD_DRV_LOG(ERR, "TX queue %u is not available or setup", >+ tx_queue_id); >+ return -EINVAL; >+ } > > /* > * tx_queue_id is queue id application refers to, while >-- >2.9.5 >