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 <ajit.khapa...@broadcom.com>
<...>
> --- 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?
<...>
