From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 78B343DC for ; Tue, 29 Aug 2017 18:16:17 +0200 (CEST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Aug 2017 09:16:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,445,1498546800"; d="scan'208";a="123704251" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by orsmga004.jf.intel.com with ESMTP; 29 Aug 2017 09:16:15 -0700 To: Ajit Khaparde , dev@dpdk.org References: <20170824162956.62761-1-ajit.khaparde@broadcom.com> <20170824162956.62761-10-ajit.khaparde@broadcom.com> From: Ferruh Yigit Message-ID: <1fefcf30-465b-b8f5-818f-9f4ba50e9224@intel.com> Date: Tue, 29 Aug 2017 17:16:14 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170824162956.62761-10-ajit.khaparde@broadcom.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 8/8] net/bnxt: add support for flow filter ops 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: , X-List-Received-Date: Tue, 29 Aug 2017 16:16:18 -0000 On 8/24/2017 5:29 PM, Ajit Khaparde wrote: > This patch adds support for flow validate/create/destroy/flush ops. Can you please update feature file [1] to document flow API support and for supported filters. Also I believe this worth mentioning in release notes [2]. [1] doc/guides/nics/features/bnxt.ini [2] doc/guides/rel_notes/release_17_11.rst > Signed-off-by: Ajit Khaparde <...> > --- a/drivers/net/bnxt/bnxt_ethdev.c > +++ b/drivers/net/bnxt/bnxt_ethdev.c > @@ -610,7 +610,7 @@ static void bnxt_mac_addr_remove_op(struct rte_eth_dev *eth_dev, > if (filter->mac_index == index) { > STAILQ_REMOVE(&vnic->filter, filter, > bnxt_filter_info, next); > - bnxt_hwrm_clear_filter(bp, filter); > + bnxt_hwrm_clear_l2_filter(bp, filter); Is it possible to extract these function name changes into different patch? <...> > + default: > + RTE_LOG(ERR, PMD, "Unknown Flow type"); > + use_ntuple |= 1; > + //return -1; Please avoid dead code. "break" ? > + } > + item++; > + } > + return use_ntuple; > +} > + <...> > + switch (item->type) { > + case RTE_FLOW_ITEM_TYPE_ETH: > + //filter_TYPE = EM can be removed. <...> > + } /* > + * else { > + * RTE_LOG(ERR, PMD, "Handle this condition\n"); > + * } > + */ Please remove unsued code. <...> > + rc = bnxt_flow_parse_attr(attr, error); > + if (rc != 0) > + goto ret; > + //Since we support ingress attribute only - right now. Please prefer /* */ comments. <...> > + filter->flags |= > + HWRM_CFA_NTUPLE_FILTER_ALLOC_INPUT_FLAGS_DROP; > + //HWRM_CFA_L2_FILTER_CFG_INPUT_FLAGS_DROP; > + //HWRM_CFA_L2_FILTER_ALLOC_INPUT_FLAGS_DROP; > + //HWRM_CFA_NTUPLE_FILTER_ALLOC_INPUT_FLAGS_DROP > + //HWRM_CFA_EM_FLOW_ALLOC_INPUT_FLAGS_DROP > + //HWRM_CFA_FLOW_ALLOC_INPUT_ACTION_FLAGS_DROP These can go. <...> > +free_flow: > + RTE_LOG(ERR, PMD, "Failed to create flow.\n"); > + rte_flow_error_set(error, -ret, > + RTE_FLOW_ERROR_TYPE_HANDLE, NULL, > + "Failed to create flow."); > + rte_free(flow); > + flow = NULL; > + return flow; return NULL; ? > +static int > +bnxt_flow_destroy(struct rte_eth_dev *dev, > + struct rte_flow *flow, > + struct rte_flow_error *error) > +{ > + struct bnxt *bp = (struct bnxt *)dev->data->dev_private; > + struct bnxt_filter_info *filter = flow->filter; > + int ret = 0; > + > + if (filter->filter_type == HWRM_CFA_EM_FILTER) > + ret = bnxt_hwrm_clear_em_filter(bp, filter); > + if (filter->filter_type == HWRM_CFA_NTUPLE_FILTER) > + ret = bnxt_hwrm_clear_ntuple_filter(bp, filter); > + > + if (!ret) { > + TAILQ_REMOVE(&bp->flow_list, flow, node); > + rte_free(flow); > + } else { > + rte_flow_error_set(error, -ret, > + RTE_FLOW_ERROR_TYPE_HANDLE, NULL, > + "Failed to destroy flow."); > + } > + > + return ret; > +} > + > +static int > +bnxt_flow_flush(struct rte_eth_dev *dev, struct rte_flow_error *error) > +{ > + struct bnxt *bp = (struct bnxt *)dev->data->dev_private; > + struct rte_flow *flow; > + int ret = 0; > + void *temp; > + > + TAILQ_FOREACH_SAFE(flow, &bp->flow_list, node, temp) { > + struct bnxt_filter_info *filter = flow->filter; > + > + if (filter->filter_type == HWRM_CFA_EM_FILTER) > + ret = bnxt_hwrm_clear_em_filter(bp, filter); > + if (filter->filter_type == HWRM_CFA_NTUPLE_FILTER) > + ret = bnxt_hwrm_clear_ntuple_filter(bp, filter); > + > + if (ret) { > + rte_flow_error_set(error, -ret, > + RTE_FLOW_ERROR_TYPE_HANDLE, NULL, > + "Failed to flush flow in HW."); > + return -rte_errno; > + } > + > + TAILQ_REMOVE(&bp->flow_list, flow, node); > + rte_free(flow); This part looks like duplication of bnxt_flow_destroy() > + } > + > + return ret; > +} <...> > diff --git a/drivers/net/bnxt/bnxt_rxq.c b/drivers/net/bnxt/bnxt_rxq.c > index 0793820..da53e99 100644 > --- a/drivers/net/bnxt/bnxt_rxq.c > +++ b/drivers/net/bnxt/bnxt_rxq.c > @@ -98,7 +98,7 @@ int bnxt_mq_rx_configure(struct bnxt *bp) > } > > /* Multi-queue mode */ > - if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_VMDQ_FLAG) { > + if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_VMDQ_DCB_RSS) { Is this change related? > /* VMDq ONLY, VMDq+RSS, VMDq+DCB, VMDq+DCB+RSS */ > enum rte_eth_nb_pools pools; > > @@ -113,6 +113,9 @@ int bnxt_mq_rx_configure(struct bnxt *bp) > pools = conf->nb_queue_pools; > break; > } > + case ETH_MQ_RX_RSS: > + pools = 1; //bp->rx_cp_nr_rings; > + break; > default: > RTE_LOG(ERR, PMD, "Unsupported mq_mod %d\n", > dev_conf->rxmode.mq_mode); > @@ -203,12 +206,42 @@ int bnxt_mq_rx_configure(struct bnxt *bp) > } > STAILQ_INSERT_TAIL(&vnic->filter, filter, next); > > - if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG) > - vnic->hash_type = > - HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_IPV4 | > - HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_IPV6; > - > out: > + if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG) { > + struct rte_eth_rss_conf *rss = &dev_conf->rx_adv_conf.rss_conf; > + uint16_t hash_type = 0; > + > + if (rss->rss_hf & ETH_RSS_IPV4) > + hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_IPV4; > + if (rss->rss_hf & ETH_RSS_NONFRAG_IPV4_TCP) > + hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_TCP_IPV4; > + if (rss->rss_hf & ETH_RSS_NONFRAG_IPV4_UDP) > + hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_UDP_IPV4; > + if (rss->rss_hf & ETH_RSS_IPV6) > + hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_IPV6; > + if (rss->rss_hf & ETH_RSS_NONFRAG_IPV6_TCP) > + hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_TCP_IPV6; > + if (rss->rss_hf & ETH_RSS_NONFRAG_IPV6_UDP) > + hash_type |= HWRM_VNIC_RSS_CFG_INPUT_HASH_TYPE_UDP_IPV6; > + > + for (i = 0; i < bp->nr_vnics; i++) { > + STAILQ_FOREACH(vnic, &bp->ff_pool[i], next) { > + vnic->hash_type |= hash_type; > + > + /* > + * Use the supplied key if the key length is > + * acceptable and the rss_key is not NULL > + */ > + if (rss->rss_key && > + rss->rss_key_len <= HW_HASH_KEY_SIZE) > + memcpy(vnic->rss_hash_key, > + rss->rss_key, rss->rss_key_len); > + } > + } > + RTE_LOG(ERR, PMD, > + "VNIC rss hash key_len %d\n", rss->rss_key_len); > + } > + Same for above, these looks like logically unrelated, can be separated into another patch? <...>